HID: playstation: DS4: Add BT poll interval adjustment

Between v6.1 and v6.2 versions of Linux kernel,
DualShock4 related code was moved from `hid-sony.c`
to `hid-playstation.c` (near DualSense code) and
Bluetooth poll interval adjustment functionality was lost

Signed-off-by: Vadym Tytan <titanv3585@gmail.com>
---
 CREDITS                       |  4 +++
 drivers/hid/hid-playstation.c | 64 +++++++++++++++++++++++++++++++++--
 2 files changed, 65 insertions(+), 3 deletions(-)

Hi Vadym,

kernel test robot noticed the following build warnings:

[auto build test WARNING on hid/for-next]
[also build test WARNING on linus/master v6.15-rc5 next-20250509]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Vadym-Tytan/HID-playstation-DS4-Add-BT-poll-interval-adjustment/20250509-054413
base:   https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git for-next
patch link:    https://lore.kernel.org/r/20250508214305.836406-1-titanv3585%40gmail.com
patch subject: [PATCH] HID: playstation: DS4: Add BT poll interval adjustment
config: x86_64-randconfig-122-20250509 (https://download.01.org/0day-ci/archive/20250510/202505100535.vKH3zHW6-lkp@intel.com/config)
compiler: clang version 20.1.2 (https://github.com/llvm/llvm-project 58df0ef89dd64126512e4ee27b4ac3fd8ddf6247)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250510/202505100535.vKH3zHW6-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202505100535.vKH3zHW6-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/hid/hid-playstation.c:1773:25: sparse: sparse: symbol 'dev_attr_dualshock4_bt_poll_interval' was not declared. Should it be static?

vim +/dev_attr_dualshock4_bt_poll_interval +1773 drivers/hid/hid-playstation.c

  1772	
> 1773	struct device_attribute dev_attr_dualshock4_bt_poll_interval = {
  1774		.attr	= { .name = "bt_poll_interval", .mode = 0644 },
  1775		.show	= dualshock4_show_poll_interval,
  1776		.store	= dualshock4_store_poll_interval,
  1777	};
  1778

> kernel test robot noticed the following build warnings:
>
> sparse warnings: (new ones prefixed by >>)
>>> drivers/hid/hid-playstation.c:1773:25: sparse: sparse: symbol 'dev_attr_dualshock4_bt_poll_interval' was not declared. Should it be static?
> 
> vim +/dev_attr_dualshock4_bt_poll_interval +1773 drivers/hid/hid-playstation.c
> 
>    1772	
>> 1773	struct device_attribute dev_attr_dualshock4_bt_poll_interval = {
>    1774		.attr	= { .name = "bt_poll_interval", .mode = 0644 },
>    1775		.show	= dualshock4_show_poll_interval,
>    1776		.store	= dualshock4_store_poll_interval,
>    1777	};
>    1778	

This piece of code is expanded `DEVICE_ATTR` macro with changed variable 
name.
`DEVICE_ATTR` is defined as:
```c
// Reference: 
https://github.com/intel-lab-lkp/linux/blob/Vadym-Tytan/HID-playstation-DS4-Add-BT-poll-interval-adjustment/20250509-054413/include/linux/device.h#L138-L158
#define DEVICE_ATTR(_name, _mode, _show, _store) \
	struct device_attribute dev_attr_##_name = __ATTR(_name, _mode, _show, 
_store)
```
It doesn't use `static` so neither did I.

P.S. Macro was expanded and variable name was changed from 
`dev_attr_bt_poll_interval` to `dev_attr_dualshock4_bt_poll_interval` 
because this attribute only applies to DualShock4

Hi,

Sorry for the late reply (business travel / vacation). This is the
only code I left out during hid-playstation creation. At the time when
I added this to hid-sony it felt like a good idea. But we later found
out the console wasn't even using this code itself. We haven't had a
need for it in various embedded projects. The code had significant
impact on battery (especially when set to like 1ms). The timing itself
was not that stable, it made it better from the hardware side
(measured using a BT packet sniffer), but there is overhead in
bluetooth stacks, which makes the timing not that predictable

Do you have a strong use case or reason to add it back?

Thanks,
Roderick

On Thu, May 8, 2025 at 2:43 PM Vadym Tytan <titanv3585@gmail.com> wrote:
>
> Between v6.1 and v6.2 versions of Linux kernel,
> DualShock4 related code was moved from `hid-sony.c`
> to `hid-playstation.c` (near DualSense code) and
> Bluetooth poll interval adjustment functionality was lost
>
> Signed-off-by: Vadym Tytan <titanv3585@gmail.com>
> ---
>  CREDITS                       |  4 +++
>  drivers/hid/hid-playstation.c | 64 +++++++++++++++++++++++++++++++++--
>  2 files changed, 65 insertions(+), 3 deletions(-)
>
> diff --git a/CREDITS b/CREDITS
> index f74d230992d6..137c3636e318 100644
> --- a/CREDITS
> +++ b/CREDITS
> @@ -4001,6 +4001,10 @@ S: 44 Campbell Park Crescent
>  S: Edinburgh EH13 0HT
>  S: United Kingdom
>
> +N: Vadym Tytan
> +E: titanv3585@gmail.com
> +D: Minor game controllers features
> +
>  N: Thomas Uhl
>  E: uhl@sun1.rz.fh-heilbronn.de
>  D: Application programmer
> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> index 1468fb11e39d..fd51f00b0516 100644
> --- a/drivers/hid/hid-playstation.c
> +++ b/drivers/hid/hid-playstation.c
> @@ -337,7 +337,7 @@ struct dualsense_output_report {
>   * 0x3F - disabled
>   */
>  #define DS4_OUTPUT_HWCTL_BT_POLL_MASK  0x3F
> -/* Default to 4ms poll interval, which is same as USB (not adjustable). */
> +/* Default to 4ms poll interval, which is same as USB (adjustable). */

The not adjustable referred to USB, which is not adjustable. So keep
that one the way it is.

>  #define DS4_BT_DEFAULT_POLL_INTERVAL_MS        4
>  #define DS4_OUTPUT_HWCTL_CRC32         0x40
>  #define DS4_OUTPUT_HWCTL_HID           0x80
> @@ -542,6 +542,7 @@ static inline void dualsense_schedule_work(struct dualsense *ds);
>  static inline void dualshock4_schedule_work(struct dualshock4 *ds4);
>  static void dualsense_set_lightbar(struct dualsense *ds, uint8_t red, uint8_t green, uint8_t blue);
>  static void dualshock4_set_default_lightbar_colors(struct dualshock4 *ds4);
> +static int dualshock4_set_bt_poll_interval(struct dualshock4 *ds4, uint8_t interval);
>
>  /*
>   * Add a new ps_device to ps_devices if it doesn't exist.
> @@ -1738,6 +1739,43 @@ static struct ps_device *dualsense_create(struct hid_device *hdev)
>         return ERR_PTR(ret);
>  }
>
> +static ssize_t dualshock4_show_poll_interval(struct device *dev,
> +                               struct device_attribute
> +                               *attr, char *buf)
> +{
> +       struct hid_device *hdev = to_hid_device(dev);
> +       struct ps_device *ps_dev = hid_get_drvdata(hdev);
> +       struct dualshock4 *ds4 = container_of(ps_dev, struct dualshock4, base);
> +
> +       return sysfs_emit(buf, "%i\n", ds4->bt_poll_interval);
> +}
> +
> +static ssize_t dualshock4_store_poll_interval(struct device *dev,
> +                               struct device_attribute *attr,
> +                               const char *buf, size_t count)
> +{
> +       struct hid_device *hdev = to_hid_device(dev);
> +       struct ps_device *ps_dev = hid_get_drvdata(hdev);
> +       struct dualshock4 *ds4 = container_of(ps_dev, struct dualshock4, base);
> +       int ret;
> +       u8 interval;
> +
> +       if (kstrtou8(buf, 0, &interval))
> +               return -EINVAL;
> +
> +       ret = dualshock4_set_bt_poll_interval(ds4, interval);
> +       if (ret)
> +               return ret;
> +
> +       return count;
> +}
> +
> +struct device_attribute dev_attr_dualshock4_bt_poll_interval = {
> +       .attr   = { .name = "bt_poll_interval", .mode = 0644 },
> +       .show   = dualshock4_show_poll_interval,
> +       .store  = dualshock4_store_poll_interval,
> +};
> +
>  static void dualshock4_dongle_calibration_work(struct work_struct *work)
>  {
>         struct dualshock4 *ds4 = container_of(work, struct dualshock4, dongle_hotplug_worker);
> @@ -2493,6 +2531,9 @@ static void dualshock4_remove(struct ps_device *ps_dev)
>         struct dualshock4 *ds4 = container_of(ps_dev, struct dualshock4, base);
>         unsigned long flags;
>
> +       if (ps_dev->hdev->bus == BUS_BLUETOOTH)
> +               device_remove_file(&ps_dev->hdev->dev, &dev_attr_dualshock4_bt_poll_interval);
> +
>         spin_lock_irqsave(&ds4->base.lock, flags);
>         ds4->output_worker_initialized = false;
>         spin_unlock_irqrestore(&ds4->base.lock, flags);
> @@ -2513,11 +2554,16 @@ static inline void dualshock4_schedule_work(struct dualshock4 *ds4)
>         spin_unlock_irqrestore(&ds4->base.lock, flags);
>  }
>
> -static void dualshock4_set_bt_poll_interval(struct dualshock4 *ds4, uint8_t interval)
> +static int dualshock4_set_bt_poll_interval(struct dualshock4 *ds4, uint8_t interval)
>  {
> +       if (interval >= DS4_OUTPUT_HWCTL_BT_POLL_MASK)
> +               return -EINVAL;
> +
>         ds4->bt_poll_interval = interval;
>         ds4->update_bt_poll_interval = true;
>         dualshock4_schedule_work(ds4);
> +
> +       return 0;
>  }
>
>  /* Set default lightbar color based on player. */
> @@ -2659,7 +2705,17 @@ static struct ps_device *dualshock4_create(struct hid_device *hdev)
>                         goto err;
>         }
>
> -       dualshock4_set_bt_poll_interval(ds4, DS4_BT_DEFAULT_POLL_INTERVAL_MS);
> +       if (hdev->bus == BUS_BLUETOOTH) {
> +               ret = dualshock4_set_bt_poll_interval(ds4, DS4_BT_DEFAULT_POLL_INTERVAL_MS);
> +               if (ret) {
> +                       hid_err(hdev, "Failed to set poll interval for DualShock4: %d\n", ret);
> +                       goto err;
> +               }
> +
> +               ret = device_create_file(&hdev->dev, &dev_attr_dualshock4_bt_poll_interval);
> +               if (ret)
> +                       hid_warn(hdev, "can't create sysfs bt_poll_interval attribute err: %d\n", ret);
> +       }
>
>         ret = ps_device_set_player_id(ps_dev);
>         if (ret) {
> @@ -2678,6 +2734,8 @@ static struct ps_device *dualshock4_create(struct hid_device *hdev)
>         return &ds4->base;
>
>  err:
> +       if (hdev->bus == BUS_BLUETOOTH)
> +               device_remove_file(&hdev->dev, &dev_attr_dualshock4_bt_poll_interval);
>         ps_devices_list_remove(ps_dev);
>         return ERR_PTR(ret);
>  }
> --
> 2.49.0
>
>

> Do you have a strong use case or reason to add it back?

I wouldn't say *strong* per se, but if something:
- Can be adjustable
   - I think if hardware is capable of such thing it's better to have 
adjustment of it available to user.
   - It's better to have an option then no option at all, let the user 
decide.
- Doesn't break the hardware nor software
   - When decreasing polling interval
     - Battery impact is just faster discharging, it doesn't "breaks" it
     - Bluetooth stack won't break, it could be just "overwhelmed"
   - When increasing polling interval
     - No negative impact at all (if we wouldn't count the user who can 
set polling interval to like 63, and have choppy controls, that's why 
defaults exists)
- Have benefits
   - PI can be decreased for smoother controls in cost of battery life
   - PI can be also increased to save battery in cost of smooth controls
- Doesn't impact anything nor anybody on defaults.

> The not adjustable referred to USB, which is not adjustable. So keep
> that one the way it is.

Oh, OK, I understood this in another way, but that make sense too.