Re: [PATCH v4 1/4] HID: add driver for Valve Steam Controller

[Andy Shevchenko]

On Thu, Mar 1, 2018 at 9:13 PM, Rodrigo Rivas Costa
 wrote:
> On Thu, Mar 01, 2018 at 12:20:37PM +0200, Andy Shevchenko wrote:
>> On Thu, Mar 1, 2018 at 12:49 AM, Rodrigo Rivas Costa
>>  wrote:
>> > On Wed, Feb 28, 2018 at 09:21:15PM +0200, Andy Shevchenko wrote:
>> >> On Wed, Feb 28, 2018 at 8:43 PM, Rodrigo Rivas Costa
>> >>  wrote:

> My bad, I didn't mean a literal free() call. More like:
>
> static void steam_unregister(struct steam_device *steam)
> {
> struct input_dev *input;
> struct power_supply *battery;
>
> rcu_read_lock();
> input = rcu_dereference(steam->input);
> battery = rcu_dereference(steam->battery);
> input_gyro = rcu_dereference(steam->input_gyro);
> rcu_read_unlock();
>

> if (input) {
> RCU_INIT_POINTER(steam->input, NULL);
> synchronize_rcu();
> hid_info(steam->hdev, "Steam Controller '%s' disconnected",
> steam->serial_no);
> input_unregister_device(input);

Candidate for a helper

static void steam_unregister_input(...)
{
...
}


steam_unregister_input(input);

> }

> if (battery) {
> RCU_INIT_POINTER(steam->battery, NULL);
> synchronize_rcu();
> power_supply_unregister(battery);
> }

Ditto.

> if (input_gyro) {
> RCU_INIT_POINTER(steam->input_gyro, NULL);
> synchronize_rcu();
> input_unregister_device(input_gyro);
> }

Ditto.

> }
>
> Also I think input_unregister_device() does not admit a NULL pointer.
> Having a special `if (!input_gyro)return;` at the end looks just
> wrong to me, it is no different from the other ones.

See above. Hide that detail in a helper function.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 1/4] HID: add driver for Valve Steam Controller

[Andy Shevchenko]

On Thu, Mar 1, 2018 at 9:13 PM, Rodrigo Rivas Costa
 wrote:
> On Thu, Mar 01, 2018 at 12:20:37PM +0200, Andy Shevchenko wrote:
>> On Thu, Mar 1, 2018 at 12:49 AM, Rodrigo Rivas Costa
>>  wrote:
>> > On Wed, Feb 28, 2018 at 09:21:15PM +0200, Andy Shevchenko wrote:
>> >> On Wed, Feb 28, 2018 at 8:43 PM, Rodrigo Rivas Costa
>> >>  wrote:

> My bad, I didn't mean a literal free() call. More like:
>
> static void steam_unregister(struct steam_device *steam)
> {
> struct input_dev *input;
> struct power_supply *battery;
>
> rcu_read_lock();
> input = rcu_dereference(steam->input);
> battery = rcu_dereference(steam->battery);
> input_gyro = rcu_dereference(steam->input_gyro);
> rcu_read_unlock();
>

> if (input) {
> RCU_INIT_POINTER(steam->input, NULL);
> synchronize_rcu();
> hid_info(steam->hdev, "Steam Controller '%s' disconnected",
> steam->serial_no);
> input_unregister_device(input);

Candidate for a helper

static void steam_unregister_input(...)
{
...
}


steam_unregister_input(input);

> }

> if (battery) {
> RCU_INIT_POINTER(steam->battery, NULL);
> synchronize_rcu();
> power_supply_unregister(battery);
> }

Ditto.

> if (input_gyro) {
> RCU_INIT_POINTER(steam->input_gyro, NULL);
> synchronize_rcu();
> input_unregister_device(input_gyro);
> }

Ditto.

> }
>
> Also I think input_unregister_device() does not admit a NULL pointer.
> Having a special `if (!input_gyro)return;` at the end looks just
> wrong to me, it is no different from the other ones.

See above. Hide that detail in a helper function.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 1/4] HID: add driver for Valve Steam Controller

[Rodrigo Rivas Costa]

On Thu, Mar 01, 2018 at 12:20:37PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 1, 2018 at 12:49 AM, Rodrigo Rivas Costa
>  wrote:
> > On Wed, Feb 28, 2018 at 09:21:15PM +0200, Andy Shevchenko wrote:
> >> On Wed, Feb 28, 2018 at 8:43 PM, Rodrigo Rivas Costa
> >>  wrote:
> 
> >> > +   if (input) {
> >>
> >> if (!input)
> >>  return;
> >>
> >> ?
> > That was because of symmetry, because further patches add more objects.
> > Then
> > if (input)
> > free(input);
> > if (battery)
> > free(battery);
> > /* in the future *(
> > if (input_gyro)
> > free(input_gyro);
> >
> > Sure, the last one can do an early return, but you break symmetry.
> 
> The generic APIs when designed properly are NULL aware at freeing resources.
> 
> So, it should look like
> 
> free(input);
> free(battery);
> free(input_gyro);

My bad, I didn't mean a literal free() call. More like:

static void steam_unregister(struct steam_device *steam)
{
struct input_dev *input;
struct power_supply *battery;

rcu_read_lock();
input = rcu_dereference(steam->input);
battery = rcu_dereference(steam->battery);
input_gyro = rcu_dereference(steam->input_gyro);
rcu_read_unlock();

if (input) {
RCU_INIT_POINTER(steam->input, NULL);
synchronize_rcu();
hid_info(steam->hdev, "Steam Controller '%s' disconnected",
steam->serial_no);
input_unregister_device(input);
}
if (battery) {
RCU_INIT_POINTER(steam->battery, NULL);
synchronize_rcu();
power_supply_unregister(battery);
}
if (input_gyro) {
RCU_INIT_POINTER(steam->input_gyro, NULL);
synchronize_rcu();
input_unregister_device(input_gyro);
}
}

Also I think input_unregister_device() does not admit a NULL pointer.
Having a special `if (!input_gyro)return;` at the end looks just
wrong to me, it is no different from the other ones.

> 
> >> > +   if (steam) {
> >> > +   cancel_work_sync(>work_connect);
> >> > +   steam_unregister(steam);
> >>
> >> > +   hid_set_drvdata(hdev, NULL);
> >>
> >> Hmm.. Doesn't HID do this?
> >
> > Do you mean the hid_set_drvdata(hdev, NULL)? I'm not sure, I didn't see
> > that on hid-core.c or hid-generic.c. And a call like this is done in
> > many modules... so I did the same, just to be sure.
> 
> What you are looking for is, AFAIU:
> 
> drivers/base/dd.c:902:  dev_set_drvdata(dev, NULL);

Ah, yes. I usually feel more comfortable setting the pointer to NULL
just after freeing the object. But currently I'm not freeing the steam
anymore, it is devm'ed, and the actual free happens from dd.c:900.

You convinced me, I'll remove that line.

> 
> >>
> >> > +   }
> >>
> >> if (steam) {
> >> ...
> >>hid_hw_stop(hdev);
> >> ...
> >> } else {
> >>hid_hw_stop(hdev);
> >> }
> >>
> >> ?
> >
> > I have no real preference. Your version has two 'if', mine has two 'if'.
> > What about:
> >
> > static void steam_remove(struct hid_device *hdev)
> > {
> > struct steam_device *steam = hid_get_drvdata(hdev);
> >
> > if (!steam) {
> > hid_hw_stop(hdev);
> > return;
> > }
> >
> > if (steam->quirks & STEAM_QUIRK_WIRELESS) {
> > hid_info(hdev, "Steam wireless receiver disconnected");
> > hid_hw_close(hdev);
> > }
> > hid_hw_stop(hdev);
> > cancel_work_sync(>work_connect);
> > steam_unregister(steam);
> 
> > hid_set_drvdata(hdev, NULL);
> 
> w/o this one.
> 
> > }
> 
> For me my version looks more compact to read, but at the end it's up to you.
> Another option split if (steam) variant into helper, and thus
> 
> if (steam)
>  steam_non_null_device_remove();
> else
>  hid_hw_stop();
> 
> But again, up to you (that's why question mark in the first place above).

Well, I don't like code such as:

if (cond) {
/* a lot of code */
} else {
/* one line */
}

because at the time I get to the else I don't remember what the
condition was about.

Regards.
Rodrigo

> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v4 1/4] HID: add driver for Valve Steam Controller

[Rodrigo Rivas Costa]

On Thu, Mar 01, 2018 at 12:20:37PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 1, 2018 at 12:49 AM, Rodrigo Rivas Costa
>  wrote:
> > On Wed, Feb 28, 2018 at 09:21:15PM +0200, Andy Shevchenko wrote:
> >> On Wed, Feb 28, 2018 at 8:43 PM, Rodrigo Rivas Costa
> >>  wrote:
> 
> >> > +   if (input) {
> >>
> >> if (!input)
> >>  return;
> >>
> >> ?
> > That was because of symmetry, because further patches add more objects.
> > Then
> > if (input)
> > free(input);
> > if (battery)
> > free(battery);
> > /* in the future *(
> > if (input_gyro)
> > free(input_gyro);
> >
> > Sure, the last one can do an early return, but you break symmetry.
> 
> The generic APIs when designed properly are NULL aware at freeing resources.
> 
> So, it should look like
> 
> free(input);
> free(battery);
> free(input_gyro);

My bad, I didn't mean a literal free() call. More like:

static void steam_unregister(struct steam_device *steam)
{
struct input_dev *input;
struct power_supply *battery;

rcu_read_lock();
input = rcu_dereference(steam->input);
battery = rcu_dereference(steam->battery);
input_gyro = rcu_dereference(steam->input_gyro);
rcu_read_unlock();

if (input) {
RCU_INIT_POINTER(steam->input, NULL);
synchronize_rcu();
hid_info(steam->hdev, "Steam Controller '%s' disconnected",
steam->serial_no);
input_unregister_device(input);
}
if (battery) {
RCU_INIT_POINTER(steam->battery, NULL);
synchronize_rcu();
power_supply_unregister(battery);
}
if (input_gyro) {
RCU_INIT_POINTER(steam->input_gyro, NULL);
synchronize_rcu();
input_unregister_device(input_gyro);
}
}

Also I think input_unregister_device() does not admit a NULL pointer.
Having a special `if (!input_gyro)return;` at the end looks just
wrong to me, it is no different from the other ones.

> 
> >> > +   if (steam) {
> >> > +   cancel_work_sync(>work_connect);
> >> > +   steam_unregister(steam);
> >>
> >> > +   hid_set_drvdata(hdev, NULL);
> >>
> >> Hmm.. Doesn't HID do this?
> >
> > Do you mean the hid_set_drvdata(hdev, NULL)? I'm not sure, I didn't see
> > that on hid-core.c or hid-generic.c. And a call like this is done in
> > many modules... so I did the same, just to be sure.
> 
> What you are looking for is, AFAIU:
> 
> drivers/base/dd.c:902:  dev_set_drvdata(dev, NULL);

Ah, yes. I usually feel more comfortable setting the pointer to NULL
just after freeing the object. But currently I'm not freeing the steam
anymore, it is devm'ed, and the actual free happens from dd.c:900.

You convinced me, I'll remove that line.

> 
> >>
> >> > +   }
> >>
> >> if (steam) {
> >> ...
> >>hid_hw_stop(hdev);
> >> ...
> >> } else {
> >>hid_hw_stop(hdev);
> >> }
> >>
> >> ?
> >
> > I have no real preference. Your version has two 'if', mine has two 'if'.
> > What about:
> >
> > static void steam_remove(struct hid_device *hdev)
> > {
> > struct steam_device *steam = hid_get_drvdata(hdev);
> >
> > if (!steam) {
> > hid_hw_stop(hdev);
> > return;
> > }
> >
> > if (steam->quirks & STEAM_QUIRK_WIRELESS) {
> > hid_info(hdev, "Steam wireless receiver disconnected");
> > hid_hw_close(hdev);
> > }
> > hid_hw_stop(hdev);
> > cancel_work_sync(>work_connect);
> > steam_unregister(steam);
> 
> > hid_set_drvdata(hdev, NULL);
> 
> w/o this one.
> 
> > }
> 
> For me my version looks more compact to read, but at the end it's up to you.
> Another option split if (steam) variant into helper, and thus
> 
> if (steam)
>  steam_non_null_device_remove();
> else
>  hid_hw_stop();
> 
> But again, up to you (that's why question mark in the first place above).

Well, I don't like code such as:

if (cond) {
/* a lot of code */
} else {
/* one line */
}

because at the time I get to the else I don't remember what the
condition was about.

Regards.
Rodrigo

> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v4 1/4] HID: add driver for Valve Steam Controller

[Andy Shevchenko]

On Thu, Mar 1, 2018 at 12:49 AM, Rodrigo Rivas Costa
 wrote:
> On Wed, Feb 28, 2018 at 09:21:15PM +0200, Andy Shevchenko wrote:
>> On Wed, Feb 28, 2018 at 8:43 PM, Rodrigo Rivas Costa
>>  wrote:

>> > +   if (input) {
>>
>> if (!input)
>>  return;
>>
>> ?
> That was because of symmetry, because further patches add more objects.
> Then
> if (input)
> free(input);
> if (battery)
> free(battery);
> /* in the future *(
> if (input_gyro)
> free(input_gyro);
>
> Sure, the last one can do an early return, but you break symmetry.

The generic APIs when designed properly are NULL aware at freeing resources.

So, it should look like

free(input);
free(battery);
free(input_gyro);


>> > +   if (steam) {
>> > +   cancel_work_sync(>work_connect);
>> > +   steam_unregister(steam);
>>
>> > +   hid_set_drvdata(hdev, NULL);
>>
>> Hmm.. Doesn't HID do this?
>
> Do you mean the hid_set_drvdata(hdev, NULL)? I'm not sure, I didn't see
> that on hid-core.c or hid-generic.c. And a call like this is done in
> many modules... so I did the same, just to be sure.

What you are looking for is, AFAIU:

drivers/base/dd.c:902:  dev_set_drvdata(dev, NULL);


>>
>> > +   }
>>
>> if (steam) {
>> ...
>>hid_hw_stop(hdev);
>> ...
>> } else {
>>hid_hw_stop(hdev);
>> }
>>
>> ?
>
> I have no real preference. Your version has two 'if', mine has two 'if'.
> What about:
>
> static void steam_remove(struct hid_device *hdev)
> {
> struct steam_device *steam = hid_get_drvdata(hdev);
>
> if (!steam) {
> hid_hw_stop(hdev);
> return;
> }
>
> if (steam->quirks & STEAM_QUIRK_WIRELESS) {
> hid_info(hdev, "Steam wireless receiver disconnected");
> hid_hw_close(hdev);
> }
> hid_hw_stop(hdev);
> cancel_work_sync(>work_connect);
> steam_unregister(steam);

> hid_set_drvdata(hdev, NULL);

w/o this one.

> }

For me my version looks more compact to read, but at the end it's up to you.
Another option split if (steam) variant into helper, and thus

if (steam)
 steam_non_null_device_remove();
else
 hid_hw_stop();

But again, up to you (that's why question mark in the first place above).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 1/4] HID: add driver for Valve Steam Controller

[Andy Shevchenko]

On Thu, Mar 1, 2018 at 12:49 AM, Rodrigo Rivas Costa
 wrote:
> On Wed, Feb 28, 2018 at 09:21:15PM +0200, Andy Shevchenko wrote:
>> On Wed, Feb 28, 2018 at 8:43 PM, Rodrigo Rivas Costa
>>  wrote:

>> > +   if (input) {
>>
>> if (!input)
>>  return;
>>
>> ?
> That was because of symmetry, because further patches add more objects.
> Then
> if (input)
> free(input);
> if (battery)
> free(battery);
> /* in the future *(
> if (input_gyro)
> free(input_gyro);
>
> Sure, the last one can do an early return, but you break symmetry.

The generic APIs when designed properly are NULL aware at freeing resources.

So, it should look like

free(input);
free(battery);
free(input_gyro);


>> > +   if (steam) {
>> > +   cancel_work_sync(>work_connect);
>> > +   steam_unregister(steam);
>>
>> > +   hid_set_drvdata(hdev, NULL);
>>
>> Hmm.. Doesn't HID do this?
>
> Do you mean the hid_set_drvdata(hdev, NULL)? I'm not sure, I didn't see
> that on hid-core.c or hid-generic.c. And a call like this is done in
> many modules... so I did the same, just to be sure.

What you are looking for is, AFAIU:

drivers/base/dd.c:902:  dev_set_drvdata(dev, NULL);


>>
>> > +   }
>>
>> if (steam) {
>> ...
>>hid_hw_stop(hdev);
>> ...
>> } else {
>>hid_hw_stop(hdev);
>> }
>>
>> ?
>
> I have no real preference. Your version has two 'if', mine has two 'if'.
> What about:
>
> static void steam_remove(struct hid_device *hdev)
> {
> struct steam_device *steam = hid_get_drvdata(hdev);
>
> if (!steam) {
> hid_hw_stop(hdev);
> return;
> }
>
> if (steam->quirks & STEAM_QUIRK_WIRELESS) {
> hid_info(hdev, "Steam wireless receiver disconnected");
> hid_hw_close(hdev);
> }
> hid_hw_stop(hdev);
> cancel_work_sync(>work_connect);
> steam_unregister(steam);

> hid_set_drvdata(hdev, NULL);

w/o this one.

> }

For me my version looks more compact to read, but at the end it's up to you.
Another option split if (steam) variant into helper, and thus

if (steam)
 steam_non_null_device_remove();
else
 hid_hw_stop();

But again, up to you (that's why question mark in the first place above).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 1/4] HID: add driver for Valve Steam Controller

[Marcus Folkesson]

Rodrigo,

On Wed, Feb 28, 2018 at 11:49:26PM +0100, Rodrigo Rivas Costa wrote:
> On Wed, Feb 28, 2018 at 09:21:15PM +0200, Andy Shevchenko wrote:
> > On Wed, Feb 28, 2018 at 8:43 PM, Rodrigo Rivas Costa
> >  wrote:
> > > There are two ways to connect the Steam Controller: directly to the USB
> > > or with the USB wireless adapter.  Both methods are similar, but the
> > > wireless adapter can connect up to 4 devices at the same time.
> > >
> > > The wired device will appear as 3 interfaces: a virtual mouse, a virtual
> > > keyboard and a custom HID device.
> > >
> > > The wireless device will appear as 5 interfaces: a virtual keyboard and
> > > 4 custom HID devices, that will remain silent until a device is actually
> > > connected.
> > >
> > > The custom HID device has a report descriptor with all vendor specific
> > > usages, so the hid-generic is not very useful. In a PC/SteamBox Valve
> > > Steam Client provices a software translation by using direct USB access
> > > and a creates a uinput virtual gamepad.
> > >
> > > This driver was reverse engineered to provide direct kernel support in
> > > case you cannot, or do not want to, use Valve Steam Client. It disables
> > > the virtual keyboard and mouse, as they are not so useful when you have
> > > a working gamepad.
> > 
> > 
> > > +// SPDX-License-Identifier: GPL-2.0
> > 
> > > +MODULE_LICENSE("GPL");
> > 
> > Not the same.
> 
> Hmmm... I copied from usb-skeleton.c, IIRC...
> I'll change to GPL-2.0+, that would be correct, I think.

Yep, the usb-skeleton.c is wrong.
I have prepared a patch, just not submitted it yet..

GPL-2.0+ is "GPLv2 or later" if that is what you want.

Best regards
Marcus Folkesson

Re: [PATCH v4 1/4] HID: add driver for Valve Steam Controller

[Marcus Folkesson]

Rodrigo,

On Wed, Feb 28, 2018 at 11:49:26PM +0100, Rodrigo Rivas Costa wrote:
> On Wed, Feb 28, 2018 at 09:21:15PM +0200, Andy Shevchenko wrote:
> > On Wed, Feb 28, 2018 at 8:43 PM, Rodrigo Rivas Costa
> >  wrote:
> > > There are two ways to connect the Steam Controller: directly to the USB
> > > or with the USB wireless adapter.  Both methods are similar, but the
> > > wireless adapter can connect up to 4 devices at the same time.
> > >
> > > The wired device will appear as 3 interfaces: a virtual mouse, a virtual
> > > keyboard and a custom HID device.
> > >
> > > The wireless device will appear as 5 interfaces: a virtual keyboard and
> > > 4 custom HID devices, that will remain silent until a device is actually
> > > connected.
> > >
> > > The custom HID device has a report descriptor with all vendor specific
> > > usages, so the hid-generic is not very useful. In a PC/SteamBox Valve
> > > Steam Client provices a software translation by using direct USB access
> > > and a creates a uinput virtual gamepad.
> > >
> > > This driver was reverse engineered to provide direct kernel support in
> > > case you cannot, or do not want to, use Valve Steam Client. It disables
> > > the virtual keyboard and mouse, as they are not so useful when you have
> > > a working gamepad.
> > 
> > 
> > > +// SPDX-License-Identifier: GPL-2.0
> > 
> > > +MODULE_LICENSE("GPL");
> > 
> > Not the same.
> 
> Hmmm... I copied from usb-skeleton.c, IIRC...
> I'll change to GPL-2.0+, that would be correct, I think.

Yep, the usb-skeleton.c is wrong.
I have prepared a patch, just not submitted it yet..

GPL-2.0+ is "GPLv2 or later" if that is what you want.

Best regards
Marcus Folkesson

Re: [PATCH v4 1/4] HID: add driver for Valve Steam Controller

[Rodrigo Rivas Costa]

On Wed, Feb 28, 2018 at 09:21:15PM +0200, Andy Shevchenko wrote:
> On Wed, Feb 28, 2018 at 8:43 PM, Rodrigo Rivas Costa
>  wrote:
> > There are two ways to connect the Steam Controller: directly to the USB
> > or with the USB wireless adapter.  Both methods are similar, but the
> > wireless adapter can connect up to 4 devices at the same time.
> >
> > The wired device will appear as 3 interfaces: a virtual mouse, a virtual
> > keyboard and a custom HID device.
> >
> > The wireless device will appear as 5 interfaces: a virtual keyboard and
> > 4 custom HID devices, that will remain silent until a device is actually
> > connected.
> >
> > The custom HID device has a report descriptor with all vendor specific
> > usages, so the hid-generic is not very useful. In a PC/SteamBox Valve
> > Steam Client provices a software translation by using direct USB access
> > and a creates a uinput virtual gamepad.
> >
> > This driver was reverse engineered to provide direct kernel support in
> > case you cannot, or do not want to, use Valve Steam Client. It disables
> > the virtual keyboard and mouse, as they are not so useful when you have
> > a working gamepad.
> 
> 
> > +// SPDX-License-Identifier: GPL-2.0
> 
> > +MODULE_LICENSE("GPL");
> 
> Not the same.

Hmmm... I copied from usb-skeleton.c, IIRC...
I'll change to GPL-2.0+, that would be correct, I think.

> 
> > +static void steam_unregister(struct steam_device *steam)
> > +{
> > +   struct input_dev *input;
> > +
> > +   rcu_read_lock();
> > +   input = rcu_dereference(steam->input);
> > +   rcu_read_unlock();
> > +
> 
> > +   if (input) {
> 
> if (!input)
>  return;
> 
> ?
That was because of symmetry, because further patches add more objects.
Then
if (input)
free(input);
if (battery)
free(battery);
/* in the future *(
if (input_gyro)
free(input_gyro);

Sure, the last one can do an early return, but you break symmetry.

> 
> > +   RCU_INIT_POINTER(steam->input, NULL);
> > +   synchronize_rcu();
> > +   hid_info(steam->hdev, "Steam Controller disconnected");
> > +   input_unregister_device(input);
> > +   }
> > +}
> 
> > +static bool steam_is_valve_interface(struct hid_device *hdev)
> > +{
> > +   struct hid_report_enum *rep_enum;
> > +   struct hid_report *hreport;
> > +
> > +   /*
> > +* The wired device creates 3 interfaces:
> > +*  0: emulated mouse.
> > +*  1: emulated keyboard.
> > +*  2: the real game pad.
> > +* The wireless device creates 5 interfaces:
> > +*  0: emulated keyboard.
> > +*  1-4: slots where up to 4 real game pads will be connected to.
> > +* We know which one is the real gamepad interface because they are 
> > the
> > +* only ones with a feature report.
> > +*/
> > +   rep_enum = >report_enum[HID_FEATURE_REPORT];
> 
> > +   list_for_each_entry(hreport, _enum->report_list, list) {
> > +   /* should we check hreport->id == 0? */
> > +   return true;
> > +   }
> > +   return false;
> 
> So, for now it's just an equivalent of
> 
> return !list_empty();
> 
> ?

I was expecting to add a few more checks in the middle, but those
weren't needed at the end.
I'll change that.

> 
> > +}
> 
> > +   /*
> > +* From this point on, if anything fails return 0 and ignores
> > +* the error, so that the default HID devices are still bound.
> > +*/
> > +   steam = devm_kzalloc(>dev,
> > +   sizeof(struct steam_device), GFP_KERNEL);
> 
> sizeof(*steam) saves a line.

Right, changed.

> 
> > +   if (!steam) {
> > +   ret = -ENOMEM;
> > +   goto mem_fail;
> > +   }
> 
> > +static void steam_remove(struct hid_device *hdev)
> > +{
> > +   struct steam_device *steam = hid_get_drvdata(hdev);
> > +
> > +   if (steam && (steam->quirks & STEAM_QUIRK_WIRELESS)) {
> > +   hid_info(hdev, "Steam wireless receiver disconnected");
> > +   hid_hw_close(hdev);
> > +   }
> > +
> > +   hid_hw_stop(hdev);
> > +
> > +   if (steam) {
> > +   cancel_work_sync(>work_connect);
> > +   steam_unregister(steam);
> 
> > +   hid_set_drvdata(hdev, NULL);
> 
> Hmm.. Doesn't HID do this?

Do you mean the hid_set_drvdata(hdev, NULL)? I'm not sure, I didn't see
that on hid-core.c or hid-generic.c. And a call like this is done in
many modules... so I did the same, just to be sure.
> 
> > +   }
> 
> if (steam) {
> ...
>hid_hw_stop(hdev);
> ...
> } else {
>hid_hw_stop(hdev);
> }
> 
> ?

I have no real preference. Your version has two 'if', mine has two 'if'.
What about:

static void steam_remove(struct hid_device *hdev)
{
struct steam_device *steam = hid_get_drvdata(hdev);

if (!steam) {
hid_hw_stop(hdev);
return;
   

Re: [PATCH v4 1/4] HID: add driver for Valve Steam Controller

[Rodrigo Rivas Costa]

On Wed, Feb 28, 2018 at 09:21:15PM +0200, Andy Shevchenko wrote:
> On Wed, Feb 28, 2018 at 8:43 PM, Rodrigo Rivas Costa
>  wrote:
> > There are two ways to connect the Steam Controller: directly to the USB
> > or with the USB wireless adapter.  Both methods are similar, but the
> > wireless adapter can connect up to 4 devices at the same time.
> >
> > The wired device will appear as 3 interfaces: a virtual mouse, a virtual
> > keyboard and a custom HID device.
> >
> > The wireless device will appear as 5 interfaces: a virtual keyboard and
> > 4 custom HID devices, that will remain silent until a device is actually
> > connected.
> >
> > The custom HID device has a report descriptor with all vendor specific
> > usages, so the hid-generic is not very useful. In a PC/SteamBox Valve
> > Steam Client provices a software translation by using direct USB access
> > and a creates a uinput virtual gamepad.
> >
> > This driver was reverse engineered to provide direct kernel support in
> > case you cannot, or do not want to, use Valve Steam Client. It disables
> > the virtual keyboard and mouse, as they are not so useful when you have
> > a working gamepad.
> 
> 
> > +// SPDX-License-Identifier: GPL-2.0
> 
> > +MODULE_LICENSE("GPL");
> 
> Not the same.

Hmmm... I copied from usb-skeleton.c, IIRC...
I'll change to GPL-2.0+, that would be correct, I think.

> 
> > +static void steam_unregister(struct steam_device *steam)
> > +{
> > +   struct input_dev *input;
> > +
> > +   rcu_read_lock();
> > +   input = rcu_dereference(steam->input);
> > +   rcu_read_unlock();
> > +
> 
> > +   if (input) {
> 
> if (!input)
>  return;
> 
> ?
That was because of symmetry, because further patches add more objects.
Then
if (input)
free(input);
if (battery)
free(battery);
/* in the future *(
if (input_gyro)
free(input_gyro);

Sure, the last one can do an early return, but you break symmetry.

> 
> > +   RCU_INIT_POINTER(steam->input, NULL);
> > +   synchronize_rcu();
> > +   hid_info(steam->hdev, "Steam Controller disconnected");
> > +   input_unregister_device(input);
> > +   }
> > +}
> 
> > +static bool steam_is_valve_interface(struct hid_device *hdev)
> > +{
> > +   struct hid_report_enum *rep_enum;
> > +   struct hid_report *hreport;
> > +
> > +   /*
> > +* The wired device creates 3 interfaces:
> > +*  0: emulated mouse.
> > +*  1: emulated keyboard.
> > +*  2: the real game pad.
> > +* The wireless device creates 5 interfaces:
> > +*  0: emulated keyboard.
> > +*  1-4: slots where up to 4 real game pads will be connected to.
> > +* We know which one is the real gamepad interface because they are 
> > the
> > +* only ones with a feature report.
> > +*/
> > +   rep_enum = >report_enum[HID_FEATURE_REPORT];
> 
> > +   list_for_each_entry(hreport, _enum->report_list, list) {
> > +   /* should we check hreport->id == 0? */
> > +   return true;
> > +   }
> > +   return false;
> 
> So, for now it's just an equivalent of
> 
> return !list_empty();
> 
> ?

I was expecting to add a few more checks in the middle, but those
weren't needed at the end.
I'll change that.

> 
> > +}
> 
> > +   /*
> > +* From this point on, if anything fails return 0 and ignores
> > +* the error, so that the default HID devices are still bound.
> > +*/
> > +   steam = devm_kzalloc(>dev,
> > +   sizeof(struct steam_device), GFP_KERNEL);
> 
> sizeof(*steam) saves a line.

Right, changed.

> 
> > +   if (!steam) {
> > +   ret = -ENOMEM;
> > +   goto mem_fail;
> > +   }
> 
> > +static void steam_remove(struct hid_device *hdev)
> > +{
> > +   struct steam_device *steam = hid_get_drvdata(hdev);
> > +
> > +   if (steam && (steam->quirks & STEAM_QUIRK_WIRELESS)) {
> > +   hid_info(hdev, "Steam wireless receiver disconnected");
> > +   hid_hw_close(hdev);
> > +   }
> > +
> > +   hid_hw_stop(hdev);
> > +
> > +   if (steam) {
> > +   cancel_work_sync(>work_connect);
> > +   steam_unregister(steam);
> 
> > +   hid_set_drvdata(hdev, NULL);
> 
> Hmm.. Doesn't HID do this?

Do you mean the hid_set_drvdata(hdev, NULL)? I'm not sure, I didn't see
that on hid-core.c or hid-generic.c. And a call like this is done in
many modules... so I did the same, just to be sure.
> 
> > +   }
> 
> if (steam) {
> ...
>hid_hw_stop(hdev);
> ...
> } else {
>hid_hw_stop(hdev);
> }
> 
> ?

I have no real preference. Your version has two 'if', mine has two 'if'.
What about:

static void steam_remove(struct hid_device *hdev)
{
struct steam_device *steam = hid_get_drvdata(hdev);

if (!steam) {
hid_hw_stop(hdev);
return;
}

if 

Re: [PATCH v4 1/4] HID: add driver for Valve Steam Controller

[Andy Shevchenko]

On Wed, Feb 28, 2018 at 8:43 PM, Rodrigo Rivas Costa
 wrote:
> There are two ways to connect the Steam Controller: directly to the USB
> or with the USB wireless adapter.  Both methods are similar, but the
> wireless adapter can connect up to 4 devices at the same time.
>
> The wired device will appear as 3 interfaces: a virtual mouse, a virtual
> keyboard and a custom HID device.
>
> The wireless device will appear as 5 interfaces: a virtual keyboard and
> 4 custom HID devices, that will remain silent until a device is actually
> connected.
>
> The custom HID device has a report descriptor with all vendor specific
> usages, so the hid-generic is not very useful. In a PC/SteamBox Valve
> Steam Client provices a software translation by using direct USB access
> and a creates a uinput virtual gamepad.
>
> This driver was reverse engineered to provide direct kernel support in
> case you cannot, or do not want to, use Valve Steam Client. It disables
> the virtual keyboard and mouse, as they are not so useful when you have
> a working gamepad.


> +// SPDX-License-Identifier: GPL-2.0

> +MODULE_LICENSE("GPL");

Not the same.

> +static void steam_unregister(struct steam_device *steam)
> +{
> +   struct input_dev *input;
> +
> +   rcu_read_lock();
> +   input = rcu_dereference(steam->input);
> +   rcu_read_unlock();
> +

> +   if (input) {

if (!input)
 return;

?

> +   RCU_INIT_POINTER(steam->input, NULL);
> +   synchronize_rcu();
> +   hid_info(steam->hdev, "Steam Controller disconnected");
> +   input_unregister_device(input);
> +   }
> +}

> +static bool steam_is_valve_interface(struct hid_device *hdev)
> +{
> +   struct hid_report_enum *rep_enum;
> +   struct hid_report *hreport;
> +
> +   /*
> +* The wired device creates 3 interfaces:
> +*  0: emulated mouse.
> +*  1: emulated keyboard.
> +*  2: the real game pad.
> +* The wireless device creates 5 interfaces:
> +*  0: emulated keyboard.
> +*  1-4: slots where up to 4 real game pads will be connected to.
> +* We know which one is the real gamepad interface because they are 
> the
> +* only ones with a feature report.
> +*/
> +   rep_enum = >report_enum[HID_FEATURE_REPORT];

> +   list_for_each_entry(hreport, _enum->report_list, list) {
> +   /* should we check hreport->id == 0? */
> +   return true;
> +   }
> +   return false;

So, for now it's just an equivalent of

return !list_empty();

?

> +}

> +   /*
> +* From this point on, if anything fails return 0 and ignores
> +* the error, so that the default HID devices are still bound.
> +*/
> +   steam = devm_kzalloc(>dev,
> +   sizeof(struct steam_device), GFP_KERNEL);

sizeof(*steam) saves a line.

> +   if (!steam) {
> +   ret = -ENOMEM;
> +   goto mem_fail;
> +   }

> +static void steam_remove(struct hid_device *hdev)
> +{
> +   struct steam_device *steam = hid_get_drvdata(hdev);
> +
> +   if (steam && (steam->quirks & STEAM_QUIRK_WIRELESS)) {
> +   hid_info(hdev, "Steam wireless receiver disconnected");
> +   hid_hw_close(hdev);
> +   }
> +
> +   hid_hw_stop(hdev);
> +
> +   if (steam) {
> +   cancel_work_sync(>work_connect);
> +   steam_unregister(steam);

> +   hid_set_drvdata(hdev, NULL);

Hmm.. Doesn't HID do this?

> +   }

if (steam) {
...
   hid_hw_stop(hdev);
...
} else {
   hid_hw_stop(hdev);
}

?

> +}

> +static void steam_do_input_event(struct steam_device *steam,
> +   struct input_dev *input, u8 *data)
> +{
> +   /* 24 bits of buttons */
> +   u8 b8, b9, b10;
> +   bool lpad_touched, lpad_and_joy;
> +
> +   b8 = data[8];
> +   b9 = data[9];
> +   b10 = data[10];
> +
> +   input_report_abs(input, ABS_Z, data[11]);
> +   input_report_abs(input, ABS_RZ, data[12]);
> +
> +   /*
> +* These two bits tells how to interpret the values X and Y.
> +* lpad_and_joy tells that the joystick and the lpad are used at the
> +* same time.
> +* lpad_touched tells whether X/Y are to be read as lpad coord or
> +* joystick values.
> +* (lpad_touched || lpad_and_joy) tells if the lpad is really touched.
> +*/

> +   lpad_touched = b10 & 0x08;

BIT(3) ?

> +   lpad_and_joy = b10 & 0x80;

BIT(7) ?

> +   input_event(input, EV_KEY, BTN_TR2, !!(b8 & 0x01));
> +   input_event(input, EV_KEY, BTN_TL2, !!(b8 & 0x02));
> +   input_event(input, EV_KEY, BTN_TR, !!(b8 & 0x04));
> +   input_event(input, EV_KEY, BTN_TL, !!(b8 & 0x08));
> +   input_event(input, EV_KEY, BTN_Y, !!(b8 & 0x10));
> +   input_event(input, EV_KEY, BTN_B, !!(b8 & 0x20));
> +   input_event(input, EV_KEY, BTN_X, 

Re: [PATCH v4 1/4] HID: add driver for Valve Steam Controller

[Andy Shevchenko]

On Wed, Feb 28, 2018 at 8:43 PM, Rodrigo Rivas Costa
 wrote:
> There are two ways to connect the Steam Controller: directly to the USB
> or with the USB wireless adapter.  Both methods are similar, but the
> wireless adapter can connect up to 4 devices at the same time.
>
> The wired device will appear as 3 interfaces: a virtual mouse, a virtual
> keyboard and a custom HID device.
>
> The wireless device will appear as 5 interfaces: a virtual keyboard and
> 4 custom HID devices, that will remain silent until a device is actually
> connected.
>
> The custom HID device has a report descriptor with all vendor specific
> usages, so the hid-generic is not very useful. In a PC/SteamBox Valve
> Steam Client provices a software translation by using direct USB access
> and a creates a uinput virtual gamepad.
>
> This driver was reverse engineered to provide direct kernel support in
> case you cannot, or do not want to, use Valve Steam Client. It disables
> the virtual keyboard and mouse, as they are not so useful when you have
> a working gamepad.


> +// SPDX-License-Identifier: GPL-2.0

> +MODULE_LICENSE("GPL");

Not the same.

> +static void steam_unregister(struct steam_device *steam)
> +{
> +   struct input_dev *input;
> +
> +   rcu_read_lock();
> +   input = rcu_dereference(steam->input);
> +   rcu_read_unlock();
> +

> +   if (input) {

if (!input)
 return;

?

> +   RCU_INIT_POINTER(steam->input, NULL);
> +   synchronize_rcu();
> +   hid_info(steam->hdev, "Steam Controller disconnected");
> +   input_unregister_device(input);
> +   }
> +}

> +static bool steam_is_valve_interface(struct hid_device *hdev)
> +{
> +   struct hid_report_enum *rep_enum;
> +   struct hid_report *hreport;
> +
> +   /*
> +* The wired device creates 3 interfaces:
> +*  0: emulated mouse.
> +*  1: emulated keyboard.
> +*  2: the real game pad.
> +* The wireless device creates 5 interfaces:
> +*  0: emulated keyboard.
> +*  1-4: slots where up to 4 real game pads will be connected to.
> +* We know which one is the real gamepad interface because they are 
> the
> +* only ones with a feature report.
> +*/
> +   rep_enum = >report_enum[HID_FEATURE_REPORT];

> +   list_for_each_entry(hreport, _enum->report_list, list) {
> +   /* should we check hreport->id == 0? */
> +   return true;
> +   }
> +   return false;

So, for now it's just an equivalent of

return !list_empty();

?

> +}

> +   /*
> +* From this point on, if anything fails return 0 and ignores
> +* the error, so that the default HID devices are still bound.
> +*/
> +   steam = devm_kzalloc(>dev,
> +   sizeof(struct steam_device), GFP_KERNEL);

sizeof(*steam) saves a line.

> +   if (!steam) {
> +   ret = -ENOMEM;
> +   goto mem_fail;
> +   }

> +static void steam_remove(struct hid_device *hdev)
> +{
> +   struct steam_device *steam = hid_get_drvdata(hdev);
> +
> +   if (steam && (steam->quirks & STEAM_QUIRK_WIRELESS)) {
> +   hid_info(hdev, "Steam wireless receiver disconnected");
> +   hid_hw_close(hdev);
> +   }
> +
> +   hid_hw_stop(hdev);
> +
> +   if (steam) {
> +   cancel_work_sync(>work_connect);
> +   steam_unregister(steam);

> +   hid_set_drvdata(hdev, NULL);

Hmm.. Doesn't HID do this?

> +   }

if (steam) {
...
   hid_hw_stop(hdev);
...
} else {
   hid_hw_stop(hdev);
}

?

> +}

> +static void steam_do_input_event(struct steam_device *steam,
> +   struct input_dev *input, u8 *data)
> +{
> +   /* 24 bits of buttons */
> +   u8 b8, b9, b10;
> +   bool lpad_touched, lpad_and_joy;
> +
> +   b8 = data[8];
> +   b9 = data[9];
> +   b10 = data[10];
> +
> +   input_report_abs(input, ABS_Z, data[11]);
> +   input_report_abs(input, ABS_RZ, data[12]);
> +
> +   /*
> +* These two bits tells how to interpret the values X and Y.
> +* lpad_and_joy tells that the joystick and the lpad are used at the
> +* same time.
> +* lpad_touched tells whether X/Y are to be read as lpad coord or
> +* joystick values.
> +* (lpad_touched || lpad_and_joy) tells if the lpad is really touched.
> +*/

> +   lpad_touched = b10 & 0x08;

BIT(3) ?

> +   lpad_and_joy = b10 & 0x80;

BIT(7) ?

> +   input_event(input, EV_KEY, BTN_TR2, !!(b8 & 0x01));
> +   input_event(input, EV_KEY, BTN_TL2, !!(b8 & 0x02));
> +   input_event(input, EV_KEY, BTN_TR, !!(b8 & 0x04));
> +   input_event(input, EV_KEY, BTN_TL, !!(b8 & 0x08));
> +   input_event(input, EV_KEY, BTN_Y, !!(b8 & 0x10));
> +   input_event(input, EV_KEY, BTN_B, !!(b8 & 0x20));
> +   input_event(input, EV_KEY, BTN_X, !!(b8 & 0x40));
> +   

[PATCH v4 1/4] HID: add driver for Valve Steam Controller

[Rodrigo Rivas Costa]

There are two ways to connect the Steam Controller: directly to the USB
or with the USB wireless adapter.  Both methods are similar, but the
wireless adapter can connect up to 4 devices at the same time.

The wired device will appear as 3 interfaces: a virtual mouse, a virtual
keyboard and a custom HID device.

The wireless device will appear as 5 interfaces: a virtual keyboard and
4 custom HID devices, that will remain silent until a device is actually
connected.

The custom HID device has a report descriptor with all vendor specific
usages, so the hid-generic is not very useful. In a PC/SteamBox Valve
Steam Client provices a software translation by using direct USB access
and a creates a uinput virtual gamepad.

This driver was reverse engineered to provide direct kernel support in
case you cannot, or do not want to, use Valve Steam Client. It disables
the virtual keyboard and mouse, as they are not so useful when you have
a working gamepad.

Working: buttons, axes, pads, wireless connect/disconnect.

TO-DO: Battery, force-feedback, accelerometer/gyro, led, beeper...

Signed-off-by: Rodrigo Rivas Costa 
---
 drivers/hid/Kconfig |   8 +
 drivers/hid/Makefile|   1 +
 drivers/hid/hid-ids.h   |   4 +
 drivers/hid/hid-steam.c | 544 
 4 files changed, 557 insertions(+)
 create mode 100644 drivers/hid/hid-steam.c

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 19c499f5623d..6e80fbf04e03 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -823,6 +823,14 @@ config HID_SPEEDLINK
---help---
Support for Speedlink Vicious and Divine Cezanne mouse.
 
+config HID_STEAM
+   tristate "Steam Controller support"
+   depends on HID
+   ---help---
+   Say Y here if you have a Steam Controller if you want to use it
+   without running the Steam Client. It supports both the wired and
+   the wireless adaptor.
+
 config HID_STEELSERIES
tristate "Steelseries SRW-S1 steering wheel support"
depends on HID
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index eb13b9e92d85..60a8abf84682 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -95,6 +95,7 @@ obj-$(CONFIG_HID_SAMSUNG) += hid-samsung.o
 obj-$(CONFIG_HID_SMARTJOYPLUS) += hid-sjoy.o
 obj-$(CONFIG_HID_SONY) += hid-sony.o
 obj-$(CONFIG_HID_SPEEDLINK)+= hid-speedlink.o
+obj-$(CONFIG_HID_STEAM)+= hid-steam.o
 obj-$(CONFIG_HID_STEELSERIES)  += hid-steelseries.o
 obj-$(CONFIG_HID_SUNPLUS)  += hid-sunplus.o
 obj-$(CONFIG_HID_GREENASIA)+= hid-gaff.o
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 43ddcdfbd0da..be31a3c20818 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -988,6 +988,10 @@
 #define USB_VENDOR_ID_STANTUM_SITRONIX 0x1403
 #define USB_DEVICE_ID_MTP_SITRONIX 0x5001
 
+#define USB_VENDOR_ID_VALVE0x28de
+#define USB_DEVICE_ID_STEAM_CONTROLLER 0x1102
+#define USB_DEVICE_ID_STEAM_CONTROLLER_WIRELESS0x1142
+
 #define USB_VENDOR_ID_STEELSERIES  0x1038
 #define USB_DEVICE_ID_STEELSERIES_SRWS10x1410
 
diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
new file mode 100644
index ..96256426c366
--- /dev/null
+++ b/drivers/hid/hid-steam.c
@@ -0,0 +1,544 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * HID driver for Valve Steam Controller
+ *
+ * Copyright (c) 2018 Rodrigo Rivas Costa 
+ *
+ * Supports both the wired and wireless interfaces.
+ *
+ * For additional functions, such as disabling the virtual mouse/keyboard or
+ * changing the right-pad margin you can use the user-space tool at:
+ *
+ *   https://github.com/rodrigorc/steamctrl
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "hid-ids.h"
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Rodrigo Rivas Costa ");
+
+#define STEAM_QUIRK_WIRELESS   BIT(0)
+
+/* Touch pads are 40 mm in diameter and 65535 units */
+#define STEAM_PAD_RESOLUTION 1638
+/* Trigger runs are about 5 mm and 256 units */
+#define STEAM_TRIGGER_RESOLUTION 51
+/* Joystick runs are about 5 mm and 256 units */
+#define STEAM_JOYSTICK_RESOLUTION 51
+
+#define STEAM_PAD_FUZZ 256
+
+struct steam_device {
+   spinlock_t lock;
+   struct hid_device *hdev;
+   struct input_dev __rcu *input;
+   unsigned long quirks;
+   struct work_struct work_connect;
+   bool connected;
+};
+
+static int steam_input_open(struct input_dev *dev)
+{
+   struct steam_device *steam = input_get_drvdata(dev);
+
+   return hid_hw_open(steam->hdev);
+}
+
+static void steam_input_close(struct input_dev *dev)
+{
+   struct steam_device *steam = input_get_drvdata(dev);
+
+   hid_hw_close(steam->hdev);
+}
+
+static int steam_register(struct steam_device *steam)
+{
+   struct hid_device *hdev = steam->hdev;
+ 

[PATCH v4 1/4] HID: add driver for Valve Steam Controller

[Rodrigo Rivas Costa]

There are two ways to connect the Steam Controller: directly to the USB
or with the USB wireless adapter.  Both methods are similar, but the
wireless adapter can connect up to 4 devices at the same time.

The wired device will appear as 3 interfaces: a virtual mouse, a virtual
keyboard and a custom HID device.

The wireless device will appear as 5 interfaces: a virtual keyboard and
4 custom HID devices, that will remain silent until a device is actually
connected.

The custom HID device has a report descriptor with all vendor specific
usages, so the hid-generic is not very useful. In a PC/SteamBox Valve
Steam Client provices a software translation by using direct USB access
and a creates a uinput virtual gamepad.

This driver was reverse engineered to provide direct kernel support in
case you cannot, or do not want to, use Valve Steam Client. It disables
the virtual keyboard and mouse, as they are not so useful when you have
a working gamepad.

Working: buttons, axes, pads, wireless connect/disconnect.

TO-DO: Battery, force-feedback, accelerometer/gyro, led, beeper...

Signed-off-by: Rodrigo Rivas Costa 
---
 drivers/hid/Kconfig |   8 +
 drivers/hid/Makefile|   1 +
 drivers/hid/hid-ids.h   |   4 +
 drivers/hid/hid-steam.c | 544 
 4 files changed, 557 insertions(+)
 create mode 100644 drivers/hid/hid-steam.c

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 19c499f5623d..6e80fbf04e03 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -823,6 +823,14 @@ config HID_SPEEDLINK
---help---
Support for Speedlink Vicious and Divine Cezanne mouse.
 
+config HID_STEAM
+   tristate "Steam Controller support"
+   depends on HID
+   ---help---
+   Say Y here if you have a Steam Controller if you want to use it
+   without running the Steam Client. It supports both the wired and
+   the wireless adaptor.
+
 config HID_STEELSERIES
tristate "Steelseries SRW-S1 steering wheel support"
depends on HID
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index eb13b9e92d85..60a8abf84682 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -95,6 +95,7 @@ obj-$(CONFIG_HID_SAMSUNG) += hid-samsung.o
 obj-$(CONFIG_HID_SMARTJOYPLUS) += hid-sjoy.o
 obj-$(CONFIG_HID_SONY) += hid-sony.o
 obj-$(CONFIG_HID_SPEEDLINK)+= hid-speedlink.o
+obj-$(CONFIG_HID_STEAM)+= hid-steam.o
 obj-$(CONFIG_HID_STEELSERIES)  += hid-steelseries.o
 obj-$(CONFIG_HID_SUNPLUS)  += hid-sunplus.o
 obj-$(CONFIG_HID_GREENASIA)+= hid-gaff.o
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 43ddcdfbd0da..be31a3c20818 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -988,6 +988,10 @@
 #define USB_VENDOR_ID_STANTUM_SITRONIX 0x1403
 #define USB_DEVICE_ID_MTP_SITRONIX 0x5001
 
+#define USB_VENDOR_ID_VALVE0x28de
+#define USB_DEVICE_ID_STEAM_CONTROLLER 0x1102
+#define USB_DEVICE_ID_STEAM_CONTROLLER_WIRELESS0x1142
+
 #define USB_VENDOR_ID_STEELSERIES  0x1038
 #define USB_DEVICE_ID_STEELSERIES_SRWS10x1410
 
diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
new file mode 100644
index ..96256426c366
--- /dev/null
+++ b/drivers/hid/hid-steam.c
@@ -0,0 +1,544 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * HID driver for Valve Steam Controller
+ *
+ * Copyright (c) 2018 Rodrigo Rivas Costa 
+ *
+ * Supports both the wired and wireless interfaces.
+ *
+ * For additional functions, such as disabling the virtual mouse/keyboard or
+ * changing the right-pad margin you can use the user-space tool at:
+ *
+ *   https://github.com/rodrigorc/steamctrl
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "hid-ids.h"
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Rodrigo Rivas Costa ");
+
+#define STEAM_QUIRK_WIRELESS   BIT(0)
+
+/* Touch pads are 40 mm in diameter and 65535 units */
+#define STEAM_PAD_RESOLUTION 1638
+/* Trigger runs are about 5 mm and 256 units */
+#define STEAM_TRIGGER_RESOLUTION 51
+/* Joystick runs are about 5 mm and 256 units */
+#define STEAM_JOYSTICK_RESOLUTION 51
+
+#define STEAM_PAD_FUZZ 256
+
+struct steam_device {
+   spinlock_t lock;
+   struct hid_device *hdev;
+   struct input_dev __rcu *input;
+   unsigned long quirks;
+   struct work_struct work_connect;
+   bool connected;
+};
+
+static int steam_input_open(struct input_dev *dev)
+{
+   struct steam_device *steam = input_get_drvdata(dev);
+
+   return hid_hw_open(steam->hdev);
+}
+
+static void steam_input_close(struct input_dev *dev)
+{
+   struct steam_device *steam = input_get_drvdata(dev);
+
+   hid_hw_close(steam->hdev);
+}
+
+static int steam_register(struct steam_device *steam)
+{
+   struct hid_device *hdev = steam->hdev;
+   struct input_dev *input;
+   int ret;
+
+   rcu_read_lock();
+   input =