* Re: [PATCH] HID: hid-steam: Add Deck IMU support
From: Jiri Kosina @ 2024-04-12 11:42 UTC (permalink / raw)
To: Max Maisel
Cc: benjamin.tissoires, linux-input, linux-kernel,
Rodrigo Rivas Costa, Vicki Pfau
In-Reply-To: <20240407121930.6012-1-mmm-1@posteo.net>
On Sun, 7 Apr 2024, Max Maisel wrote:
> The Deck's controller features an accelerometer and gyroscope which
> send their measurement values by default in the main HID input report.
> Expose both sensors to userspace through a separate evdev node as it
> is done by the hid-nintendo and hid-playstation drivers.
>
> Signed-off-by: Max Maisel <mmm-1@posteo.net>
CCing Rodrigo and Vicki ... could you please take a look and Ack the patch
below from Max?
Thanks.
> ---
>
> This patch was tested on a Steam Deck running Arch Linux. With it,
> applications using latest SDL2/3 git libraries will pick up the sensors
> without hidraw access. This was tested against the antimicrox gamepad mapper.
>
> Measurement value scaling was tested by moving the deck and a dualsense
> controller simultaneously and comparing their reported values in
> userspace with SDL3's testcontroller tool.
>
> drivers/hid/hid-steam.c | 158 ++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 150 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index b08a5ab58528..af6e6c3b1356 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
> @@ -66,6 +66,12 @@ static LIST_HEAD(steam_devices);
> #define STEAM_DECK_TRIGGER_RESOLUTION 5461
> /* Joystick runs are about 5 mm and 32768 units */
> #define STEAM_DECK_JOYSTICK_RESOLUTION 6553
> +/* Accelerometer has 16 bit resolution and a range of +/- 2g */
> +#define STEAM_DECK_ACCEL_RES_PER_G 16384
> +#define STEAM_DECK_ACCEL_RANGE 32768
> +/* Gyroscope has 16 bit resolution and a range of +/- 2000 dps */
> +#define STEAM_DECK_GYRO_RES_PER_DPS 16
> +#define STEAM_DECK_GYRO_RANGE 32000
>
> #define STEAM_PAD_FUZZ 256
>
> @@ -288,6 +294,7 @@ struct steam_device {
> struct mutex report_mutex;
> unsigned long client_opened;
> struct input_dev __rcu *input;
> + struct input_dev __rcu *sensors;
> unsigned long quirks;
> struct work_struct work_connect;
> bool connected;
> @@ -302,6 +309,7 @@ struct steam_device {
> struct work_struct rumble_work;
> u16 rumble_left;
> u16 rumble_right;
> + unsigned int sensor_timestamp_us;
> };
>
> static int steam_recv_report(struct steam_device *steam,
> @@ -825,6 +833,74 @@ static int steam_input_register(struct steam_device *steam)
> return ret;
> }
>
> +static int steam_sensors_register(struct steam_device *steam)
> +{
> + struct hid_device *hdev = steam->hdev;
> + struct input_dev *sensors;
> + int ret;
> +
> + if (!(steam->quirks & STEAM_QUIRK_DECK))
> + return 0;
> +
> + rcu_read_lock();
> + sensors = rcu_dereference(steam->sensors);
> + rcu_read_unlock();
> + if (sensors) {
> + dbg_hid("%s: already connected\n", __func__);
> + return 0;
> + }
> +
> + sensors = input_allocate_device();
> + if (!sensors)
> + return -ENOMEM;
> +
> + input_set_drvdata(sensors, steam);
> + sensors->dev.parent = &hdev->dev;
> +
> + sensors->name = "Steam Deck Motion Sensors";
> + sensors->phys = hdev->phys;
> + sensors->uniq = steam->serial_no;
> + sensors->id.bustype = hdev->bus;
> + sensors->id.vendor = hdev->vendor;
> + sensors->id.product = hdev->product;
> + sensors->id.version = hdev->version;
> +
> + __set_bit(INPUT_PROP_ACCELEROMETER, sensors->propbit);
> + __set_bit(EV_MSC, sensors->evbit);
> + __set_bit(MSC_TIMESTAMP, sensors->mscbit);
> +
> + input_set_abs_params(sensors, ABS_X, -STEAM_DECK_ACCEL_RANGE,
> + STEAM_DECK_ACCEL_RANGE, 16, 0);
> + input_set_abs_params(sensors, ABS_Y, -STEAM_DECK_ACCEL_RANGE,
> + STEAM_DECK_ACCEL_RANGE, 16, 0);
> + input_set_abs_params(sensors, ABS_Z, -STEAM_DECK_ACCEL_RANGE,
> + STEAM_DECK_ACCEL_RANGE, 16, 0);
> + input_abs_set_res(sensors, ABS_X, STEAM_DECK_ACCEL_RES_PER_G);
> + input_abs_set_res(sensors, ABS_Y, STEAM_DECK_ACCEL_RES_PER_G);
> + input_abs_set_res(sensors, ABS_Z, STEAM_DECK_ACCEL_RES_PER_G);
> +
> + input_set_abs_params(sensors, ABS_RX, -STEAM_DECK_GYRO_RANGE,
> + STEAM_DECK_GYRO_RANGE, 16, 0);
> + input_set_abs_params(sensors, ABS_RY, -STEAM_DECK_GYRO_RANGE,
> + STEAM_DECK_GYRO_RANGE, 16, 0);
> + input_set_abs_params(sensors, ABS_RZ, -STEAM_DECK_GYRO_RANGE,
> + STEAM_DECK_GYRO_RANGE, 16, 0);
> + input_abs_set_res(sensors, ABS_RX, STEAM_DECK_GYRO_RES_PER_DPS);
> + input_abs_set_res(sensors, ABS_RY, STEAM_DECK_GYRO_RES_PER_DPS);
> + input_abs_set_res(sensors, ABS_RZ, STEAM_DECK_GYRO_RES_PER_DPS);
> +
> + ret = input_register_device(sensors);
> + if (ret)
> + goto sensors_register_fail;
> +
> + rcu_assign_pointer(steam->sensors, sensors);
> + return 0;
> +
> +sensors_register_fail:
> + input_free_device(sensors);
> + return ret;
> +}
> +
> static void steam_input_unregister(struct steam_device *steam)
> {
> struct input_dev *input;
> @@ -838,6 +914,24 @@ static void steam_input_unregister(struct steam_device *steam)
> input_unregister_device(input);
> }
>
> +static void steam_sensors_unregister(struct steam_device *steam)
> +{
> + struct input_dev *sensors;
> +
> + if (!(steam->quirks & STEAM_QUIRK_DECK))
> + return;
> +
> + rcu_read_lock();
> + sensors = rcu_dereference(steam->sensors);
> + rcu_read_unlock();
> +
> + if (!sensors)
> + return;
> + RCU_INIT_POINTER(steam->sensors, NULL);
> + synchronize_rcu();
> + input_unregister_device(sensors);
> +}
> +
> static void steam_battery_unregister(struct steam_device *steam)
> {
> struct power_supply *battery;
> @@ -890,18 +984,28 @@ static int steam_register(struct steam_device *steam)
> spin_lock_irqsave(&steam->lock, flags);
> client_opened = steam->client_opened;
> spin_unlock_irqrestore(&steam->lock, flags);
> +
> if (!client_opened) {
> steam_set_lizard_mode(steam, lizard_mode);
> ret = steam_input_register(steam);
> - } else
> - ret = 0;
> + if (ret != 0)
> + goto steam_register_input_fail;
> + ret = steam_sensors_register(steam);
> + if (ret != 0)
> + goto steam_register_sensors_fail;
> + }
> + return 0;
>
> +steam_register_sensors_fail:
> + steam_input_unregister(steam);
> +steam_register_input_fail:
> return ret;
> }
>
> static void steam_unregister(struct steam_device *steam)
> {
> steam_battery_unregister(steam);
> + steam_sensors_unregister(steam);
> steam_input_unregister(steam);
> if (steam->serial_no[0]) {
> hid_info(steam->hdev, "Steam Controller '%s' disconnected",
> @@ -1010,6 +1114,7 @@ static int steam_client_ll_open(struct hid_device *hdev)
> steam->client_opened++;
> spin_unlock_irqrestore(&steam->lock, flags);
>
> + steam_sensors_unregister(steam);
> steam_input_unregister(steam);
>
> return 0;
> @@ -1030,6 +1135,7 @@ static void steam_client_ll_close(struct hid_device *hdev)
> if (connected) {
> steam_set_lizard_mode(steam, lizard_mode);
> steam_input_register(steam);
> + steam_sensors_register(steam);
> }
> }
>
> @@ -1121,6 +1227,7 @@ static int steam_probe(struct hid_device *hdev,
> INIT_DELAYED_WORK(&steam->mode_switch, steam_mode_switch_cb);
> INIT_LIST_HEAD(&steam->list);
> INIT_WORK(&steam->rumble_work, steam_haptic_rumble_cb);
> + steam->sensor_timestamp_us = 0;
>
> /*
> * With the real steam controller interface, do not connect hidraw.
> @@ -1380,12 +1487,12 @@ static void steam_do_input_event(struct steam_device *steam,
> * 18-19 | s16 | ABS_HAT0Y | left-pad Y value
> * 20-21 | s16 | ABS_HAT1X | right-pad X value
> * 22-23 | s16 | ABS_HAT1Y | right-pad Y value
> - * 24-25 | s16 | -- | accelerometer X value
> - * 26-27 | s16 | -- | accelerometer Y value
> - * 28-29 | s16 | -- | accelerometer Z value
> - * 30-31 | s16 | -- | gyro X value
> - * 32-33 | s16 | -- | gyro Y value
> - * 34-35 | s16 | -- | gyro Z value
> + * 24-25 | s16 | IMU ABS_X | accelerometer X value
> + * 26-27 | s16 | IMU ABS_Z | accelerometer Y value
> + * 28-29 | s16 | IMU ABS_Y | accelerometer Z value
> + * 30-31 | s16 | IMU ABS_RX | gyro X value
> + * 32-33 | s16 | IMU ABS_RZ | gyro Y value
> + * 34-35 | s16 | IMU ABS_RY | gyro Z value
> * 36-37 | s16 | -- | quaternion W value
> * 38-39 | s16 | -- | quaternion X value
> * 40-41 | s16 | -- | quaternion Y value
> @@ -1546,6 +1653,32 @@ static void steam_do_deck_input_event(struct steam_device *steam,
> input_sync(input);
> }
>
> +static void steam_do_deck_sensors_event(struct steam_device *steam,
> + struct input_dev *sensors, u8 *data)
> +{
> + /*
> + * The deck input report is received every 4 ms on average,
> + * with a jitter of +/- 4 ms even though the USB descriptor claims
> + * that it uses 1 kHz.
> + * Since the HID report does not include a sensor timestamp,
> + * use a fixed increment here.
> + *
> + * The reported sensors data is factory calibrated by default so
> + * no extra logic for handling calibratrion is necessary.
> + */
> + steam->sensor_timestamp_us += 4000;
> + input_event(sensors, EV_MSC, MSC_TIMESTAMP, steam->sensor_timestamp_us);
> +
> + input_report_abs(sensors, ABS_X, steam_le16(data + 24));
> + input_report_abs(sensors, ABS_Z, -steam_le16(data + 26));
> + input_report_abs(sensors, ABS_Y, steam_le16(data + 28));
> + input_report_abs(sensors, ABS_RX, steam_le16(data + 30));
> + input_report_abs(sensors, ABS_RZ, -steam_le16(data + 32));
> + input_report_abs(sensors, ABS_RY, steam_le16(data + 34));
> +
> + input_sync(sensors);
> +}
> +
> /*
> * The size for this message payload is 11.
> * The known values are:
> @@ -1583,6 +1716,7 @@ static int steam_raw_event(struct hid_device *hdev,
> {
> struct steam_device *steam = hid_get_drvdata(hdev);
> struct input_dev *input;
> + struct input_dev *sensors;
> struct power_supply *battery;
>
> if (!steam)
> @@ -1629,6 +1763,14 @@ static int steam_raw_event(struct hid_device *hdev,
> if (likely(input))
> steam_do_deck_input_event(steam, input, data);
> rcu_read_unlock();
> +
> + if (steam->quirks & STEAM_QUIRK_DECK) {
> + rcu_read_lock();
> + sensors = rcu_dereference(steam->sensors);
> + if (likely(sensors))
> + steam_do_deck_sensors_event(steam, sensors, data);
> + rcu_read_unlock();
> + }
> break;
> case ID_CONTROLLER_WIRELESS:
> /*
>
> base-commit: 39cd87c4eb2b893354f3b850f916353f2658ae6f
> --
> 2.44.0
>
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* [PATCH v10 0/4] Add support for vibrator in multiple PMICs
From: Fenglin Wu via B4 Relay @ 2024-04-12 12:36 UTC (permalink / raw)
To: kernel, Andy Gross, Bjorn Andersson, Konrad Dybcio,
Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Dmitry Baryshkov
Cc: linux-arm-msm, linux-input, linux-kernel, devicetree, Fenglin Wu,
Krzysztof Kozlowski
Add SW support for the vibrator module inside PMI632, PM7250B, PM7325B, PM7550BA.
It is very similar to the vibrator module inside PM8916 which is supported in
pm8xxx-vib driver but just the drive amplitude is controlled with 2 registers,
and the register base offset in each PMIC is different.
Changes in v10:
1. Add Fixes tag
2. Update SSBI vibrator to use DT 'reg' value
3. Add drv_in_step flag for programming vibrator level in steps
Link to v9: https://lore.kernel.org/r/20240411-pm8xxx-vibrator-new-design-v9-0-7bf56cb92b28@quicinc.com
Changes in v9:
1. Add a preceding change to correct VIB_MAX_LEVELS calculation
2. Address review comments from Konrad
Link to v8: https://lore.kernel.org/r/20240401-pm8xxx-vibrator-new-design-v8-0-6f2b8b03b4c7@quicinc.com
Changes in v8:
1. Remove hw_type, and still keep the register info in match data
2. Update to use register offset in pm8xxx_regs, and the base address
defined in DT for SPMI vibrator will be added in register access
3. Update voltage output range for SPMI vibrator which has 2 bytes drive
registers
Changes in v7:
1. Fix a typo: SSBL_VIB_DRV_REG --> SSBI_VIB_DRV_REG
2. Move the hw_type switch case in pm8xxx_vib_set() to the refactoring
change.
Changes in v6:
1. Add "qcom,pmi632-vib" as a standalone compatible string.
Changes in v5:
1. Drop "qcom,spmi-vib-gen2" generic compatible string as requested
and use device specific compatible strings only.
Changes in v4:
1. Update to use the combination of the HW type and register offset
as the constant match data, the register base address defined in
'reg' property will be added when accessing SPMI registers using
regmap APIs.
2. Remove 'qcom,spmi-vib-gen1' generic compatible string.
Changes in v3:
1. Refactor the driver to support different type of the vibrators with
better flexibility by introducing the HW type with corresponding
register fields definitions.
2. Add 'qcom,spmi-vib-gen1' and 'qcom,spmi-vib-gen2' compatible
strings, and add PMI632, PM7250B, PM7325B, PM7550BA as compatbile as
spmi-vib-gen2.
Changes in v2:
Remove the "pm7550ba-vib" compatible string as it's compatible with pm7325b.
Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
---
Fenglin Wu (4):
input: pm8xxx-vibrator: correct VIB_MAX_LEVELS calculation
input: pm8xxx-vibrator: refactor to support new SPMI vibrator
dt-bindings: input: qcom,pm8xxx-vib: add new SPMI vibrator module
input: pm8xxx-vibrator: add new SPMI vibrator support
.../devicetree/bindings/input/qcom,pm8xxx-vib.yaml | 16 +++-
drivers/input/misc/pm8xxx-vibrator.c | 95 ++++++++++++++++------
2 files changed, 84 insertions(+), 27 deletions(-)
---
base-commit: 650cda2ce25f08e8fae391b3ba6be27e7296c6a5
change-id: 20240328-pm8xxx-vibrator-new-design-e5811ad59e8a
Best regards,
--
Fenglin Wu <quic_fenglinw@quicinc.com>
^ permalink raw reply
* [PATCH v10 1/4] input: pm8xxx-vibrator: correct VIB_MAX_LEVELS calculation
From: Fenglin Wu via B4 Relay @ 2024-04-12 12:36 UTC (permalink / raw)
To: kernel, Andy Gross, Bjorn Andersson, Konrad Dybcio,
Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Dmitry Baryshkov
Cc: linux-arm-msm, linux-input, linux-kernel, devicetree, Fenglin Wu
In-Reply-To: <20240412-pm8xxx-vibrator-new-design-v10-0-0ec0ad133866@quicinc.com>
From: Fenglin Wu <quic_fenglinw@quicinc.com>
The output voltage is inclusive hence the max level calculation is
off-by-one-step. Correct it.
Fixes: 11205bb63e5c ("Input: add support for pm8xxx based vibrator driver")
Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
---
drivers/input/misc/pm8xxx-vibrator.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
index 04cb87efd799..844ca7e1f59f 100644
--- a/drivers/input/misc/pm8xxx-vibrator.c
+++ b/drivers/input/misc/pm8xxx-vibrator.c
@@ -14,7 +14,8 @@
#define VIB_MAX_LEVEL_mV (3100)
#define VIB_MIN_LEVEL_mV (1200)
-#define VIB_MAX_LEVELS (VIB_MAX_LEVEL_mV - VIB_MIN_LEVEL_mV)
+#define VIB_PER_STEP_mV (100)
+#define VIB_MAX_LEVELS (VIB_MAX_LEVEL_mV - VIB_MIN_LEVEL_mV + VIB_PER_STEP_mV)
#define MAX_FF_SPEED 0xff
@@ -118,10 +119,10 @@ static void pm8xxx_work_handler(struct work_struct *work)
vib->active = true;
vib->level = ((VIB_MAX_LEVELS * vib->speed) / MAX_FF_SPEED) +
VIB_MIN_LEVEL_mV;
- vib->level /= 100;
+ vib->level /= VIB_PER_STEP_mV;
} else {
vib->active = false;
- vib->level = VIB_MIN_LEVEL_mV / 100;
+ vib->level = VIB_MIN_LEVEL_mV / VIB_PER_STEP_mV;
}
pm8xxx_vib_set(vib, vib->active);
--
2.25.1
^ permalink raw reply related
* [PATCH v10 2/4] input: pm8xxx-vibrator: refactor to support new SPMI vibrator
From: Fenglin Wu via B4 Relay @ 2024-04-12 12:36 UTC (permalink / raw)
To: kernel, Andy Gross, Bjorn Andersson, Konrad Dybcio,
Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Dmitry Baryshkov
Cc: linux-arm-msm, linux-input, linux-kernel, devicetree, Fenglin Wu
In-Reply-To: <20240412-pm8xxx-vibrator-new-design-v10-0-0ec0ad133866@quicinc.com>
From: Fenglin Wu <quic_fenglinw@quicinc.com>
Currently, vibrator control register addresses are hard coded,
including the base address and offsets, it's not flexible to
support new SPMI vibrator module which is usually included in
different PMICs with different base address. Refactor it by using
the base address defined in devicetree.
Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
---
drivers/input/misc/pm8xxx-vibrator.c | 41 +++++++++++++++++++++++-------------
1 file changed, 26 insertions(+), 15 deletions(-)
diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
index 844ca7e1f59f..640927f94143 100644
--- a/drivers/input/misc/pm8xxx-vibrator.c
+++ b/drivers/input/misc/pm8xxx-vibrator.c
@@ -20,27 +20,27 @@
#define MAX_FF_SPEED 0xff
struct pm8xxx_regs {
- unsigned int enable_addr;
+ unsigned int enable_offset;
unsigned int enable_mask;
- unsigned int drv_addr;
+ unsigned int drv_offset;
unsigned int drv_mask;
unsigned int drv_shift;
unsigned int drv_en_manual_mask;
};
static const struct pm8xxx_regs pm8058_regs = {
- .drv_addr = 0x4A,
+ .drv_offset = 0,
.drv_mask = 0xf8,
.drv_shift = 3,
.drv_en_manual_mask = 0xfc,
};
static struct pm8xxx_regs pm8916_regs = {
- .enable_addr = 0xc046,
+ .enable_offset = 0x46,
.enable_mask = BIT(7),
- .drv_addr = 0xc041,
- .drv_mask = 0x1F,
+ .drv_offset = 0x41,
+ .drv_mask = 0x1f,
.drv_shift = 0,
.drv_en_manual_mask = 0,
};
@@ -51,6 +51,8 @@ static struct pm8xxx_regs pm8916_regs = {
* @work: work structure to set the vibration parameters
* @regmap: regmap for register read/write
* @regs: registers' info
+ * @enable_addr: vibrator enable register
+ * @drv_addr: vibrator drive strength register
* @speed: speed of vibration set from userland
* @active: state of vibrator
* @level: level of vibration to set in the chip
@@ -61,6 +63,8 @@ struct pm8xxx_vib {
struct work_struct work;
struct regmap *regmap;
const struct pm8xxx_regs *regs;
+ unsigned int enable_addr;
+ unsigned int drv_addr;
int speed;
int level;
bool active;
@@ -83,15 +87,15 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
else
val &= ~regs->drv_mask;
- rc = regmap_write(vib->regmap, regs->drv_addr, val);
+ rc = regmap_write(vib->regmap, vib->drv_addr, val);
if (rc < 0)
return rc;
vib->reg_vib_drv = val;
if (regs->enable_mask)
- rc = regmap_update_bits(vib->regmap, regs->enable_addr,
- regs->enable_mask, on ? ~0 : 0);
+ rc = regmap_update_bits(vib->regmap, vib->enable_addr,
+ regs->enable_mask, on ? regs->enable_mask : 0);
return rc;
}
@@ -103,11 +107,10 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
static void pm8xxx_work_handler(struct work_struct *work)
{
struct pm8xxx_vib *vib = container_of(work, struct pm8xxx_vib, work);
- const struct pm8xxx_regs *regs = vib->regs;
- int rc;
unsigned int val;
+ int rc;
- rc = regmap_read(vib->regmap, regs->drv_addr, &val);
+ rc = regmap_read(vib->regmap, vib->drv_addr, &val);
if (rc < 0)
return;
@@ -170,7 +173,7 @@ static int pm8xxx_vib_probe(struct platform_device *pdev)
struct pm8xxx_vib *vib;
struct input_dev *input_dev;
int error;
- unsigned int val;
+ unsigned int val, reg_base = 0;
const struct pm8xxx_regs *regs;
vib = devm_kzalloc(&pdev->dev, sizeof(*vib), GFP_KERNEL);
@@ -188,15 +191,23 @@ static int pm8xxx_vib_probe(struct platform_device *pdev)
INIT_WORK(&vib->work, pm8xxx_work_handler);
vib->vib_input_dev = input_dev;
+ error = fwnode_property_read_u32(pdev->dev.fwnode, "reg", ®_base);
+ if (error < 0) {
+ dev_err(&pdev->dev, "Failed to read reg address, rc=%d\n", error);
+ return error;
+ }
+
regs = of_device_get_match_data(&pdev->dev);
+ vib->enable_addr = reg_base + regs->enable_offset;
+ vib->drv_addr = reg_base + regs->drv_offset;
/* operate in manual mode */
- error = regmap_read(vib->regmap, regs->drv_addr, &val);
+ error = regmap_read(vib->regmap, vib->drv_addr, &val);
if (error < 0)
return error;
val &= regs->drv_en_manual_mask;
- error = regmap_write(vib->regmap, regs->drv_addr, val);
+ error = regmap_write(vib->regmap, vib->drv_addr, val);
if (error < 0)
return error;
--
2.25.1
^ permalink raw reply related
* [PATCH v10 4/4] input: pm8xxx-vibrator: add new SPMI vibrator support
From: Fenglin Wu via B4 Relay @ 2024-04-12 12:36 UTC (permalink / raw)
To: kernel, Andy Gross, Bjorn Andersson, Konrad Dybcio,
Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Dmitry Baryshkov
Cc: linux-arm-msm, linux-input, linux-kernel, devicetree, Fenglin Wu
In-Reply-To: <20240412-pm8xxx-vibrator-new-design-v10-0-0ec0ad133866@quicinc.com>
From: Fenglin Wu <quic_fenglinw@quicinc.com>
Add support for a new SPMI vibrator module which is very similar
to the vibrator module inside PM8916 but has a finer drive voltage
step and different output voltage range, its drive level control
is expanded across 2 registers. The vibrator module can be found
in following Qualcomm PMICs: PMI632, PM7250B, PM7325B, PM7550BA.
Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
---
drivers/input/misc/pm8xxx-vibrator.c | 55 ++++++++++++++++++++++++++++++------
1 file changed, 46 insertions(+), 9 deletions(-)
diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
index 640927f94143..8b9d757d650f 100644
--- a/drivers/input/misc/pm8xxx-vibrator.c
+++ b/drivers/input/misc/pm8xxx-vibrator.c
@@ -12,10 +12,11 @@
#include <linux/regmap.h>
#include <linux/slab.h>
-#define VIB_MAX_LEVEL_mV (3100)
-#define VIB_MIN_LEVEL_mV (1200)
-#define VIB_PER_STEP_mV (100)
-#define VIB_MAX_LEVELS (VIB_MAX_LEVEL_mV - VIB_MIN_LEVEL_mV + VIB_PER_STEP_mV)
+#define VIB_MAX_LEVEL_mV(vib) (vib->drv2_addr ? 3544 : 3100)
+#define VIB_MIN_LEVEL_mV(vib) (vib->drv2_addr ? 1504 : 1200)
+#define VIB_PER_STEP_mV(vib) (vib->drv2_addr ? 8 : 100)
+#define VIB_MAX_LEVELS(vib) \
+ (VIB_MAX_LEVEL_mV(vib) - VIB_MIN_LEVEL_mV(vib) + VIB_PER_STEP_mV(vib))
#define MAX_FF_SPEED 0xff
@@ -26,7 +27,11 @@ struct pm8xxx_regs {
unsigned int drv_offset;
unsigned int drv_mask;
unsigned int drv_shift;
+ unsigned int drv2_offset;
+ unsigned int drv2_mask;
+ unsigned int drv2_shift;
unsigned int drv_en_manual_mask;
+ bool drv_in_step;
};
static const struct pm8xxx_regs pm8058_regs = {
@@ -34,6 +39,7 @@ static const struct pm8xxx_regs pm8058_regs = {
.drv_mask = 0xf8,
.drv_shift = 3,
.drv_en_manual_mask = 0xfc,
+ .drv_in_step = true,
};
static struct pm8xxx_regs pm8916_regs = {
@@ -43,6 +49,20 @@ static struct pm8xxx_regs pm8916_regs = {
.drv_mask = 0x1f,
.drv_shift = 0,
.drv_en_manual_mask = 0,
+ .drv_in_step = true,
+};
+
+static struct pm8xxx_regs pmi632_regs = {
+ .enable_offset = 0x46,
+ .enable_mask = BIT(7),
+ .drv_offset = 0x40,
+ .drv_mask = GENMASK(7, 0),
+ .drv_shift = 0,
+ .drv2_offset = 0x41,
+ .drv2_mask = GENMASK(3, 0),
+ .drv2_shift = 8,
+ .drv_en_manual_mask = 0,
+ .drv_in_step = false,
};
/**
@@ -53,6 +73,7 @@ static struct pm8xxx_regs pm8916_regs = {
* @regs: registers' info
* @enable_addr: vibrator enable register
* @drv_addr: vibrator drive strength register
+ * @drv2_addr: vibrator drive strength upper byte register
* @speed: speed of vibration set from userland
* @active: state of vibrator
* @level: level of vibration to set in the chip
@@ -65,6 +86,7 @@ struct pm8xxx_vib {
const struct pm8xxx_regs *regs;
unsigned int enable_addr;
unsigned int drv_addr;
+ unsigned int drv2_addr;
int speed;
int level;
bool active;
@@ -82,6 +104,9 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
unsigned int val = vib->reg_vib_drv;
const struct pm8xxx_regs *regs = vib->regs;
+ if (regs->drv_in_step)
+ vib->level /= VIB_PER_STEP_mV(vib);
+
if (on)
val |= (vib->level << regs->drv_shift) & regs->drv_mask;
else
@@ -93,6 +118,17 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
vib->reg_vib_drv = val;
+ if (regs->drv2_mask) {
+ if (on)
+ val = (vib->level << regs->drv2_shift) & regs->drv2_mask;
+ else
+ val = 0;
+
+ rc = regmap_write_bits(vib->regmap, vib->drv2_addr, regs->drv2_mask, val);
+ if (rc < 0)
+ return rc;
+ }
+
if (regs->enable_mask)
rc = regmap_update_bits(vib->regmap, vib->enable_addr,
regs->enable_mask, on ? regs->enable_mask : 0);
@@ -115,17 +151,16 @@ static void pm8xxx_work_handler(struct work_struct *work)
return;
/*
- * pmic vibrator supports voltage ranges from 1.2 to 3.1V, so
+ * pmic vibrator supports voltage ranges from MIN_LEVEL to MAX_LEVEL, so
* scale the level to fit into these ranges.
*/
if (vib->speed) {
vib->active = true;
- vib->level = ((VIB_MAX_LEVELS * vib->speed) / MAX_FF_SPEED) +
- VIB_MIN_LEVEL_mV;
- vib->level /= VIB_PER_STEP_mV;
+ vib->level = VIB_MIN_LEVEL_mV(vib);
+ vib->level += mult_frac(VIB_MAX_LEVELS(vib), vib->speed, MAX_FF_SPEED);
} else {
vib->active = false;
- vib->level = VIB_MIN_LEVEL_mV / VIB_PER_STEP_mV;
+ vib->level = VIB_MIN_LEVEL_mV(vib);
}
pm8xxx_vib_set(vib, vib->active);
@@ -200,6 +235,7 @@ static int pm8xxx_vib_probe(struct platform_device *pdev)
regs = of_device_get_match_data(&pdev->dev);
vib->enable_addr = reg_base + regs->enable_offset;
vib->drv_addr = reg_base + regs->drv_offset;
+ vib->drv2_addr = reg_base + regs->drv2_offset;
/* operate in manual mode */
error = regmap_read(vib->regmap, vib->drv_addr, &val);
@@ -254,6 +290,7 @@ static const struct of_device_id pm8xxx_vib_id_table[] = {
{ .compatible = "qcom,pm8058-vib", .data = &pm8058_regs },
{ .compatible = "qcom,pm8921-vib", .data = &pm8058_regs },
{ .compatible = "qcom,pm8916-vib", .data = &pm8916_regs },
+ { .compatible = "qcom,pmi632-vib", .data = &pmi632_regs },
{ }
};
MODULE_DEVICE_TABLE(of, pm8xxx_vib_id_table);
--
2.25.1
^ permalink raw reply related
* [PATCH v10 3/4] dt-bindings: input: qcom,pm8xxx-vib: add new SPMI vibrator module
From: Fenglin Wu via B4 Relay @ 2024-04-12 12:36 UTC (permalink / raw)
To: kernel, Andy Gross, Bjorn Andersson, Konrad Dybcio,
Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Dmitry Baryshkov
Cc: linux-arm-msm, linux-input, linux-kernel, devicetree, Fenglin Wu,
Krzysztof Kozlowski
In-Reply-To: <20240412-pm8xxx-vibrator-new-design-v10-0-0ec0ad133866@quicinc.com>
From: Fenglin Wu <quic_fenglinw@quicinc.com>
Add compatible strings to support vibrator module inside PMI632,
PMI7250B, PM7325B, PM7550BA.
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
---
.../devicetree/bindings/input/qcom,pm8xxx-vib.yaml | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml b/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml
index c8832cd0d7da..2025d6a5423e 100644
--- a/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml
+++ b/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml
@@ -11,10 +11,18 @@ maintainers:
properties:
compatible:
- enum:
- - qcom,pm8058-vib
- - qcom,pm8916-vib
- - qcom,pm8921-vib
+ oneOf:
+ - enum:
+ - qcom,pm8058-vib
+ - qcom,pm8916-vib
+ - qcom,pm8921-vib
+ - qcom,pmi632-vib
+ - items:
+ - enum:
+ - qcom,pm7250b-vib
+ - qcom,pm7325b-vib
+ - qcom,pm7550ba-vib
+ - const: qcom,pmi632-vib
reg:
maxItems: 1
--
2.25.1
^ permalink raw reply related
* Re: [bug report] HID: playstation: DS4: Don't fail on calibration data request
From: Max Staudt @ 2024-04-12 14:57 UTC (permalink / raw)
To: Dan Carpenter, Jiri Kosina
Cc: linux-input, linux-kernel, Roderick Colenbrander
In-Reply-To: <07848795-14e3-4020-9e60-e3221ff6ef80@enpas.org>
On 4/8/24 23:44, Max Staudt wrote:
> On 4/8/24 09:41, Dan Carpenter wrote:
>> Hello Max Staudt,
>>
>> Commit a48a7cd85f55 ("HID: playstation: DS4: Don't fail on
>> calibration data request") from Feb 8, 2024 (linux-next), leads to
>> the following Smatch static checker warning:
>>
>> [...]
>
>
> Hi Dan, Jiri,
>
> Thanks for the report!
>
>
> Jiri, if you prefer to do so, please feel free to stop/revert this patch for now, and I'll send a better one soon.
>
> [...]
Jiri,
Would you like me to send a patch on top of the existing one, or a completely fresh one?
Max
^ permalink raw reply
* Re: [bug report] HID: playstation: DS4: Don't fail on calibration data request
From: Jiri Kosina @ 2024-04-12 15:01 UTC (permalink / raw)
To: Max Staudt
Cc: Dan Carpenter, linux-input, linux-kernel, Roderick Colenbrander
In-Reply-To: <4e486902-9238-48db-b0b2-2abce4f3b812@enpas.org>
On Fri, 12 Apr 2024, Max Staudt wrote:
> >> Commit a48a7cd85f55 ("HID: playstation: DS4: Don't fail on
> >> calibration data request") from Feb 8, 2024 (linux-next), leads to
> >> the following Smatch static checker warning:
> >>
> >> [...]
> >
> >
> > Hi Dan, Jiri,
> >
> > Thanks for the report!
> >
> >
> > Jiri, if you prefer to do so, please feel free to stop/revert this
> > patch for now, and I'll send a better one soon.
> >
> > [...]
>
> Jiri,
>
> Would you like me to send a patch on top of the existing one, or a completely
> fresh one?
Max,
please send a followup one with proper Fixes: tag. We're generally not
rebasing the tree.
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [bug report] HID: playstation: DS4: Don't fail on calibration data request
From: Max Staudt @ 2024-04-12 15:23 UTC (permalink / raw)
To: Jiri Kosina
Cc: Dan Carpenter, linux-input, linux-kernel, Roderick Colenbrander
In-Reply-To: <nycvar.YFH.7.76.2404121700540.5680@cbobk.fhfr.pm>
On 4/13/24 00:01, Jiri Kosina wrote:
> Max,
>
> please send a followup one with proper Fixes: tag. We're generally not
> rebasing the tree.
Instead of rebasing, I was wondering whether you were intending to revert the faulty patch first :)
Thanks, will do!
Max
^ permalink raw reply
* Re: [PATCH] HID: mcp-2221: cancel delayed_work only when CONFIG_IIO is enabled
From: Jiri Kosina @ 2024-04-12 15:49 UTC (permalink / raw)
To: Abdelrahman Morsy
Cc: linux-input, linux-i2c, linux-kernel, gupt21, benjamin.tissoires
In-Reply-To: <20240402121406.1703500-1-abdelrahmanhesham94@gmail.com>
On Tue, 2 Apr 2024, Abdelrahman Morsy wrote:
> If the device is unplugged and CONFIG_IIO is not supported, this will
> result in a warning message at kernel/workqueue.
>
> Only cancel delayed work in mcp2221_remove(), when CONFIG_IIO is enabled.
Makes sense, the work is scheduled also only with CONFIG_IIO enabled.
Thanks, applied.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] HID: uclogic: Remove useless loop
From: Jiri Kosina @ 2024-04-12 15:52 UTC (permalink / raw)
To: Stefan Berzl; +Cc: linux-input, linux-kernel, Nikolai Kondrashov
In-Reply-To: <20240401004757.22708-1-stefanberzl@gmail.com>
On Mon, 1 Apr 2024, Stefan Berzl wrote:
> The while in question does nothing except provide the possibility
> to have an infinite loop in case the subreport id is actually the same
> as the pen id.
>
> Signed-off-by: Stefan Berzl <stefanberzl@gmail.com>
Let me CC Nicolai, the author of the code of question (8b013098be2c9).
> ---
> drivers/hid/hid-uclogic-core.c | 55 ++++++++++++++++------------------
> 1 file changed, 25 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/hid/hid-uclogic-core.c b/drivers/hid/hid-uclogic-core.c
> index ad74cbc9a0aa..a56f4de216de 100644
> --- a/drivers/hid/hid-uclogic-core.c
> +++ b/drivers/hid/hid-uclogic-core.c
> @@ -431,40 +431,35 @@ static int uclogic_raw_event(struct hid_device *hdev,
> if (uclogic_exec_event_hook(params, data, size))
> return 0;
>
> - while (true) {
> - /* Tweak pen reports, if necessary */
> - if ((report_id == params->pen.id) && (size >= 2)) {
> - subreport_list_end =
> - params->pen.subreport_list +
> - ARRAY_SIZE(params->pen.subreport_list);
> - /* Try to match a subreport */
> - for (subreport = params->pen.subreport_list;
> - subreport < subreport_list_end; subreport++) {
> - if (subreport->value != 0 &&
> - subreport->value == data[1]) {
> - break;
> - }
> - }
> - /* If a subreport matched */
> - if (subreport < subreport_list_end) {
> - /* Change to subreport ID, and restart */
> - report_id = data[0] = subreport->id;
> - continue;
> - } else {
> - return uclogic_raw_event_pen(drvdata, data, size);
> + /* Tweak pen reports, if necessary */
> + if ((report_id == params->pen.id) && (size >= 2)) {
> + subreport_list_end =
> + params->pen.subreport_list +
> + ARRAY_SIZE(params->pen.subreport_list);
> + /* Try to match a subreport */
> + for (subreport = params->pen.subreport_list;
> + subreport < subreport_list_end; subreport++) {
> + if (subreport->value != 0 &&
> + subreport->value == data[1]) {
> + break;
> }
> }
> -
> - /* Tweak frame control reports, if necessary */
> - for (i = 0; i < ARRAY_SIZE(params->frame_list); i++) {
> - if (report_id == params->frame_list[i].id) {
> - return uclogic_raw_event_frame(
> - drvdata, ¶ms->frame_list[i],
> - data, size);
> - }
> + /* If a subreport matched */
> + if (subreport < subreport_list_end) {
> + /* Change to subreport ID, and restart */
> + report_id = data[0] = subreport->id;
> + } else {
> + return uclogic_raw_event_pen(drvdata, data, size);
> }
> + }
>
> - break;
> + /* Tweak frame control reports, if necessary */
> + for (i = 0; i < ARRAY_SIZE(params->frame_list); i++) {
> + if (report_id == params->frame_list[i].id) {
> + return uclogic_raw_event_frame(
> + drvdata, ¶ms->frame_list[i],
> + data, size);
> + }
> }
>
> return 0;
> --
> 2.43.0
>
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH][next] HID: sony: remove redundant assignment to variable ret
From: Jiri Kosina @ 2024-04-12 15:56 UTC (permalink / raw)
To: Colin Ian King
Cc: Benjamin Tissoires, linux-input, kernel-janitors, linux-kernel
In-Reply-To: <20240328122213.762889-1-colin.i.king@gmail.com>
On Thu, 28 Mar 2024, Colin Ian King wrote:
> The variable ret is being assigned a value that is never read
> afterwards. The assignment is redundant and can be removed.
>
> Cleans up clang scan build warning:
> drivers/hid/hid-sony.c:2020:3: warning: Value stored to 'ret'
> is never read [deadcode.DeadStores]
That assignment is indeed bogus. Applied, thanks.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH v10 1/4] input: pm8xxx-vibrator: correct VIB_MAX_LEVELS calculation
From: Dmitry Baryshkov @ 2024-04-12 16:21 UTC (permalink / raw)
To: quic_fenglinw
Cc: kernel, Andy Gross, Bjorn Andersson, Konrad Dybcio,
Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, linux-arm-msm,
linux-input, linux-kernel, devicetree
In-Reply-To: <20240412-pm8xxx-vibrator-new-design-v10-1-0ec0ad133866@quicinc.com>
On Fri, 12 Apr 2024 at 15:36, Fenglin Wu via B4 Relay
<devnull+quic_fenglinw.quicinc.com@kernel.org> wrote:
>
> From: Fenglin Wu <quic_fenglinw@quicinc.com>
>
> The output voltage is inclusive hence the max level calculation is
> off-by-one-step. Correct it.
... while we are at it also add a define for the step size instead of
using the magic value.
With that in place:
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>
> Fixes: 11205bb63e5c ("Input: add support for pm8xxx based vibrator driver")
> Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
> ---
> drivers/input/misc/pm8xxx-vibrator.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
> index 04cb87efd799..844ca7e1f59f 100644
> --- a/drivers/input/misc/pm8xxx-vibrator.c
> +++ b/drivers/input/misc/pm8xxx-vibrator.c
> @@ -14,7 +14,8 @@
>
> #define VIB_MAX_LEVEL_mV (3100)
> #define VIB_MIN_LEVEL_mV (1200)
> -#define VIB_MAX_LEVELS (VIB_MAX_LEVEL_mV - VIB_MIN_LEVEL_mV)
> +#define VIB_PER_STEP_mV (100)
> +#define VIB_MAX_LEVELS (VIB_MAX_LEVEL_mV - VIB_MIN_LEVEL_mV + VIB_PER_STEP_mV)
>
> #define MAX_FF_SPEED 0xff
>
> @@ -118,10 +119,10 @@ static void pm8xxx_work_handler(struct work_struct *work)
> vib->active = true;
> vib->level = ((VIB_MAX_LEVELS * vib->speed) / MAX_FF_SPEED) +
> VIB_MIN_LEVEL_mV;
> - vib->level /= 100;
> + vib->level /= VIB_PER_STEP_mV;
> } else {
> vib->active = false;
> - vib->level = VIB_MIN_LEVEL_mV / 100;
> + vib->level = VIB_MIN_LEVEL_mV / VIB_PER_STEP_mV;
> }
>
> pm8xxx_vib_set(vib, vib->active);
>
> --
> 2.25.1
>
>
--
With best wishes
Dmitry
^ permalink raw reply
* Re: hid-logitech-dj support for Anywhere 3SB
From: Filipe Laíns @ 2024-04-12 19:53 UTC (permalink / raw)
To: Allan Sandfeld Jensen, linux-input
In-Reply-To: <4887001.GXAFRqVoOG@twilight>
[-- Attachment #1: Type: text/plain, Size: 2898 bytes --]
On Fri, 2024-04-12 at 11:10 +0200, Allan Sandfeld Jensen wrote:
> Hello,
>
> I am writing because you are listed as author of the hid-logitech-dj driver. I
> recently bought a Logitech Anywhere 3SB mouse, and found Linux didn't
> recognize it. Thinking it was a simple case of new IDs not recognized, I
> quickly added them to the logitch HID++ drivers (patch attached), both for USB
> with the new receiver and for the Bluetooth direct connection.
>
> I have noticed however that the patch while causing them to be recognized and
> interacted with as HID++ devices, it has a flaw. The scroll wheel events are
> reported by the linux kernel as being in hires mode, while haven't actually
> enabled it on the mouse. You can fix that using Solaar, but some piece is
> missing to enable it correctly in the driver. Since this is no longer a
> trivial fix. I wanted to reach out. Do you have any suggestions?
>
> Best regards
> Allan
Hi Allan,
Thank you for reaching out.
What likely is happening here is Solaar overwriting the configuration that the
kernel driver sets, as that would happen after the driver talks to the device.
The settings in question need support in both the kernel and the userspace input
stack (libinput) for them to work appropriately, it's not like configuring RGB
or other sort setting on the device that works standalone.
I have, multiple times now, asked for Solaar to not expose these low level
settings that need support from other parts of the input stack, leaving them to
the kernel to configure.
I have been inactive in the Solaar project for quite some time now, so I don't
feel like yet again make a big deal out of this there, so that this decision is
reconsidered. I have already spent a significant amount of effort there, and
nowadays I barely have energy to go through my day and deal with my all my
responsibilities and other OSS project involvements, so I sadly have no more
energy to spare there.
My recommendation is: disable Solaar from running at startup, restart the
system, and see if that solves your problem. If it does, report this issue again
to the Solaar upstream, then depending on that outcome, make a decision on how
to proceed. It may be that setting the high-resolution settings in Solaar, which
are expected by the driver, works, but it might not be super reliable, because
since Solaar is overwriting the settings configured by the kernel driver, if
anything in the kernel driver changes, then the setting you have configured in
Solaar might no longer be correct.
There are alternatives to Solaar that do not have this issue, like libratbag,
but these generally are feature lacking on the productivity line of Logitech
projects.
Sorry I wasn't able to help much, but I hope that this clarifies things a bit,
and helps you solve your problem.
Cheers,
Filipe Laíns
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH 0/6] input: use device_for_each_child_node_scoped()
From: Javier Carrasco @ 2024-04-12 20:57 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel, Javier Carrasco
Switch to the _scoped() version introduced in commit 365130fd47af
("device property: Introduce device_for_each_child_node_scoped()")
to remove the need for manual calling of fwnode_handle_put() in the
paths where the code exits the loop early. This modification simplifies
the code and eliminates the risk of leaking memory if any early exit is
added without de-allocating the child node.
There are six users of the non-scoped version in the input subsystem:
- iqs269a
- qt1050
- gpio_keys
- gpio_keys_polled
- adc-keys
- adc-joystick
This series is based on the master branch of linux-next (next-20240412)
to have access to the scoped version of device_for_each_child_node().
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
Javier Carrasco (6):
input: iqs269a: use device_for_each_child_node_scoped()
input: qt1050: use device_for_each_child_node_scoped()
input: gpio_keys: use device_for_each_child_node_scoped()
input: gpio_keys_polled: use device_for_each_child_node_scoped()
input: adc-keys: use device_for_each_child_node_scoped()
input: adc-joystick: use device_for_each_child_node_scoped()
drivers/input/joystick/adc-joystick.c | 16 +++++-----------
drivers/input/keyboard/adc-keys.c | 5 +----
drivers/input/keyboard/gpio_keys.c | 4 +---
drivers/input/keyboard/gpio_keys_polled.c | 4 +---
drivers/input/keyboard/qt1050.c | 12 ++++--------
drivers/input/misc/iqs269a.c | 7 ++-----
6 files changed, 14 insertions(+), 34 deletions(-)
---
base-commit: 9ed46da14b9b9b2ad4edb3b0c545b6dbe5c00d39
change-id: 20240404-input_device_for_each_child_node_scoped-0a55a76ad7ee
Best regards,
--
Javier Carrasco <javier.carrasco.cruz@gmail.com>
^ permalink raw reply
* [PATCH 1/6] input: iqs269a: use device_for_each_child_node_scoped()
From: Javier Carrasco @ 2024-04-12 20:57 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel, Javier Carrasco
In-Reply-To: <20240412-input_device_for_each_child_node_scoped-v1-0-dbad1bc7ea84@gmail.com>
Switch to the _scoped() version introduced in commit 365130fd47af
("device property: Introduce device_for_each_child_node_scoped()")
to remove the need for manual calling of fwnode_handle_put() in the
paths where the code exits the loop early.
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
drivers/input/misc/iqs269a.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/input/misc/iqs269a.c b/drivers/input/misc/iqs269a.c
index cd14ff9f57cf..843f8a3f3410 100644
--- a/drivers/input/misc/iqs269a.c
+++ b/drivers/input/misc/iqs269a.c
@@ -811,7 +811,6 @@ static int iqs269_parse_prop(struct iqs269_private *iqs269)
{
struct iqs269_sys_reg *sys_reg = &iqs269->sys_reg;
struct i2c_client *client = iqs269->client;
- struct fwnode_handle *ch_node;
u16 general, misc_a, misc_b;
unsigned int val;
int error;
@@ -1049,12 +1048,10 @@ static int iqs269_parse_prop(struct iqs269_private *iqs269)
sys_reg->event_mask = ~((u8)IQS269_EVENT_MASK_SYS);
- device_for_each_child_node(&client->dev, ch_node) {
+ device_for_each_child_node_scoped(&client->dev, ch_node) {
error = iqs269_parse_chan(iqs269, ch_node);
- if (error) {
- fwnode_handle_put(ch_node);
+ if (error)
return error;
- }
}
/*
--
2.40.1
^ permalink raw reply related
* [PATCH 2/6] input: qt1050: use device_for_each_child_node_scoped()
From: Javier Carrasco @ 2024-04-12 20:57 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel, Javier Carrasco
In-Reply-To: <20240412-input_device_for_each_child_node_scoped-v1-0-dbad1bc7ea84@gmail.com>
Switch to the _scoped() version introduced in commit 365130fd47af
("device property: Introduce device_for_each_child_node_scoped()")
to remove the need for manual calling of fwnode_handle_put() in the
paths where the code exits the loop early.
In this case the err label was no longer necessary and EINVAL is
returned directly.
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
drivers/input/keyboard/qt1050.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/input/keyboard/qt1050.c b/drivers/input/keyboard/qt1050.c
index b51dfcd76038..6ac2b9dbdb85 100644
--- a/drivers/input/keyboard/qt1050.c
+++ b/drivers/input/keyboard/qt1050.c
@@ -355,21 +355,21 @@ static int qt1050_parse_fw(struct qt1050_priv *ts)
if (fwnode_property_read_u32(child, "linux,code",
&button.keycode)) {
dev_err(dev, "Button without keycode\n");
- goto err;
+ return -EINVAL;
}
if (button.keycode >= KEY_MAX) {
dev_err(dev, "Invalid keycode 0x%x\n",
button.keycode);
- goto err;
+ return -EINVAL;
}
if (fwnode_property_read_u32(child, "reg",
&button.num)) {
dev_err(dev, "Button without pad number\n");
- goto err;
+ return -EINVAL;
}
if (button.num < 0 || button.num > QT1050_MAX_KEYS - 1)
- goto err;
+ return -EINVAL;
ts->reg_keys |= BIT(button.num);
@@ -419,10 +419,6 @@ static int qt1050_parse_fw(struct qt1050_priv *ts)
}
return 0;
-
-err:
- fwnode_handle_put(child);
- return -EINVAL;
}
static int qt1050_probe(struct i2c_client *client)
--
2.40.1
^ permalink raw reply related
* [PATCH 3/6] input: gpio_keys: use device_for_each_child_node_scoped()
From: Javier Carrasco @ 2024-04-12 20:57 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel, Javier Carrasco
In-Reply-To: <20240412-input_device_for_each_child_node_scoped-v1-0-dbad1bc7ea84@gmail.com>
Switch to the _scoped() version introduced in commit 365130fd47af
("device property: Introduce device_for_each_child_node_scoped()")
to remove the need for manual calling of fwnode_handle_put() in the
paths where the code exits the loop early.
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
drivers/input/keyboard/gpio_keys.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 9f3bcd41cf67..9fb0bdcfbf9e 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -768,7 +768,6 @@ gpio_keys_get_devtree_pdata(struct device *dev)
{
struct gpio_keys_platform_data *pdata;
struct gpio_keys_button *button;
- struct fwnode_handle *child;
int nbuttons, irq;
nbuttons = device_get_child_node_count(dev);
@@ -790,7 +789,7 @@ gpio_keys_get_devtree_pdata(struct device *dev)
device_property_read_string(dev, "label", &pdata->name);
- device_for_each_child_node(dev, child) {
+ device_for_each_child_node_scoped(dev, child) {
if (is_of_node(child)) {
irq = of_irq_get_byname(to_of_node(child), "irq");
if (irq > 0)
@@ -808,7 +807,6 @@ gpio_keys_get_devtree_pdata(struct device *dev)
if (fwnode_property_read_u32(child, "linux,code",
&button->code)) {
dev_err(dev, "Button without keycode\n");
- fwnode_handle_put(child);
return ERR_PTR(-EINVAL);
}
--
2.40.1
^ permalink raw reply related
* [PATCH 4/6] input: gpio_keys_polled: use device_for_each_child_node_scoped()
From: Javier Carrasco @ 2024-04-12 20:57 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel, Javier Carrasco
In-Reply-To: <20240412-input_device_for_each_child_node_scoped-v1-0-dbad1bc7ea84@gmail.com>
Switch to the _scoped() version introduced in commit 365130fd47af
("device property: Introduce device_for_each_child_node_scoped()")
to remove the need for manual calling of fwnode_handle_put() in the
paths where the code exits the loop early.
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
drivers/input/keyboard/gpio_keys_polled.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/input/keyboard/gpio_keys_polled.c b/drivers/input/keyboard/gpio_keys_polled.c
index b41fd1240f43..41ca0d3c9098 100644
--- a/drivers/input/keyboard/gpio_keys_polled.c
+++ b/drivers/input/keyboard/gpio_keys_polled.c
@@ -144,7 +144,6 @@ gpio_keys_polled_get_devtree_pdata(struct device *dev)
{
struct gpio_keys_platform_data *pdata;
struct gpio_keys_button *button;
- struct fwnode_handle *child;
int nbuttons;
nbuttons = device_get_child_node_count(dev);
@@ -166,11 +165,10 @@ gpio_keys_polled_get_devtree_pdata(struct device *dev)
device_property_read_string(dev, "label", &pdata->name);
- device_for_each_child_node(dev, child) {
+ device_for_each_child_node_scoped(dev, child) {
if (fwnode_property_read_u32(child, "linux,code",
&button->code)) {
dev_err(dev, "button without keycode\n");
- fwnode_handle_put(child);
return ERR_PTR(-EINVAL);
}
--
2.40.1
^ permalink raw reply related
* [PATCH 5/6] input: adc-keys: use device_for_each_child_node_scoped()
From: Javier Carrasco @ 2024-04-12 20:57 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel, Javier Carrasco
In-Reply-To: <20240412-input_device_for_each_child_node_scoped-v1-0-dbad1bc7ea84@gmail.com>
Switch to the _scoped() version introduced in commit 365130fd47af
("device property: Introduce device_for_each_child_node_scoped()")
to remove the need for manual calling of fwnode_handle_put() in the
paths where the code exits the loop early.
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
drivers/input/keyboard/adc-keys.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/input/keyboard/adc-keys.c b/drivers/input/keyboard/adc-keys.c
index bf72ab8df817..f1753207429d 100644
--- a/drivers/input/keyboard/adc-keys.c
+++ b/drivers/input/keyboard/adc-keys.c
@@ -66,7 +66,6 @@ static void adc_keys_poll(struct input_dev *input)
static int adc_keys_load_keymap(struct device *dev, struct adc_keys_state *st)
{
struct adc_keys_button *map;
- struct fwnode_handle *child;
int i;
st->num_keys = device_get_child_node_count(dev);
@@ -80,11 +79,10 @@ static int adc_keys_load_keymap(struct device *dev, struct adc_keys_state *st)
return -ENOMEM;
i = 0;
- device_for_each_child_node(dev, child) {
+ device_for_each_child_node_scoped(dev, child) {
if (fwnode_property_read_u32(child, "press-threshold-microvolt",
&map[i].voltage)) {
dev_err(dev, "Key with invalid or missing voltage\n");
- fwnode_handle_put(child);
return -EINVAL;
}
map[i].voltage /= 1000;
@@ -92,7 +90,6 @@ static int adc_keys_load_keymap(struct device *dev, struct adc_keys_state *st)
if (fwnode_property_read_u32(child, "linux,code",
&map[i].keycode)) {
dev_err(dev, "Key with invalid or missing linux,code\n");
- fwnode_handle_put(child);
return -EINVAL;
}
--
2.40.1
^ permalink raw reply related
* [PATCH 6/6] input: adc-joystick: use device_for_each_child_node_scoped()
From: Javier Carrasco @ 2024-04-12 20:57 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel, Javier Carrasco
In-Reply-To: <20240412-input_device_for_each_child_node_scoped-v1-0-dbad1bc7ea84@gmail.com>
Switch to the _scoped() version introduced in commit 365130fd47af
("device property: Introduce device_for_each_child_node_scoped()")
to remove the need for manual calling of fwnode_handle_put() in the
paths where the code exits the loop early.
In this case the err_fwnode_put label was no longer necessary and the
error code is returned directly.
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
drivers/input/joystick/adc-joystick.c | 16 +++++-----------
1 file changed, 5 insertions(+), 11 deletions(-)
diff --git a/drivers/input/joystick/adc-joystick.c b/drivers/input/joystick/adc-joystick.c
index c0deff5d4282..c7c2edf908e6 100644
--- a/drivers/input/joystick/adc-joystick.c
+++ b/drivers/input/joystick/adc-joystick.c
@@ -122,7 +122,6 @@ static void adc_joystick_cleanup(void *data)
static int adc_joystick_set_axes(struct device *dev, struct adc_joystick *joy)
{
struct adc_joystick_axis *axes;
- struct fwnode_handle *child;
int num_axes, error, i;
num_axes = device_get_child_node_count(dev);
@@ -141,31 +140,30 @@ static int adc_joystick_set_axes(struct device *dev, struct adc_joystick *joy)
if (!axes)
return -ENOMEM;
- device_for_each_child_node(dev, child) {
+ device_for_each_child_node_scoped(dev, child) {
error = fwnode_property_read_u32(child, "reg", &i);
if (error) {
dev_err(dev, "reg invalid or missing\n");
- goto err_fwnode_put;
+ return error;
}
if (i >= num_axes) {
- error = -EINVAL;
dev_err(dev, "No matching axis for reg %d\n", i);
- goto err_fwnode_put;
+ return -EINVAL;
}
error = fwnode_property_read_u32(child, "linux,code",
&axes[i].code);
if (error) {
dev_err(dev, "linux,code invalid or missing\n");
- goto err_fwnode_put;
+ return error;
}
error = fwnode_property_read_u32_array(child, "abs-range",
axes[i].range, 2);
if (error) {
dev_err(dev, "abs-range invalid or missing\n");
- goto err_fwnode_put;
+ return error;
}
fwnode_property_read_u32(child, "abs-fuzz", &axes[i].fuzz);
@@ -180,10 +178,6 @@ static int adc_joystick_set_axes(struct device *dev, struct adc_joystick *joy)
joy->axes = axes;
return 0;
-
-err_fwnode_put:
- fwnode_handle_put(child);
- return error;
}
static int adc_joystick_probe(struct platform_device *pdev)
--
2.40.1
^ permalink raw reply related
* Re: hid-logitech-dj support for Anywhere 3SB
From: Allan Sandfeld Jensen @ 2024-04-13 8:20 UTC (permalink / raw)
To: linux-input, Filipe Laíns
In-Reply-To: <fade9f2881f9b69c0bb3f3b63463c8e9b7656871.camel@riseup.net>
On Friday 12 April 2024 21:53:31 CEST Filipe Laíns wrote:
> On Fri, 2024-04-12 at 11:10 +0200, Allan Sandfeld Jensen wrote:
> > Hello,
> >
> > I am writing because you are listed as author of the hid-logitech-dj
> > driver. I recently bought a Logitech Anywhere 3SB mouse, and found Linux
> > didn't recognize it. Thinking it was a simple case of new IDs not
> > recognized, I quickly added them to the logitch HID++ drivers (patch
> > attached), both for USB with the new receiver and for the Bluetooth
> > direct connection.
> >
> > I have noticed however that the patch while causing them to be recognized
> > and interacted with as HID++ devices, it has a flaw. The scroll wheel
> > events are reported by the linux kernel as being in hires mode, while
> > haven't actually enabled it on the mouse. You can fix that using Solaar,
> > but some piece is missing to enable it correctly in the driver. Since
> > this is no longer a trivial fix. I wanted to reach out. Do you have any
> > suggestions?
> >
> > Best regards
> > Allan
>
> Hi Allan,
>
> Thank you for reaching out.
>
> What likely is happening here is Solaar overwriting the configuration that
> the kernel driver sets, as that would happen after the driver talks to the
> device.
>
> The settings in question need support in both the kernel and the userspace
> input stack (libinput) for them to work appropriately, it's not like
> configuring RGB or other sort setting on the device that works standalone.
We already have the support in the kernel and libinput. That is why I am
expanding it to recognize this new device id.(?)
> I have, multiple times now, asked for Solaar to not expose these low level
> settings that need support from other parts of the input stack, leaving them
> to the kernel to configure.
> I have been inactive in the Solaar project for quite some time now, so I
> don't feel like yet again make a big deal out of this there, so that this
> decision is reconsidered. I have already spent a significant amount of
> effort there, and nowadays I barely have energy to go through my day and
> deal with my all my responsibilities and other OSS project involvements, so
> I sadly have no more energy to spare there.
>
> My recommendation is: disable Solaar from running at startup, restart the
> system, and see if that solves your problem. If it does, report this issue
> again to the Solaar upstream, then depending on that outcome, make a
> decision on how to proceed. It may be that setting the high-resolution
> settings in Solaar, which are expected by the driver, works, but it might
> not be super reliable, because since Solaar is overwriting the settings
> configured by the kernel driver, if anything in the kernel driver changes,
> then the setting you have configured in Solaar might no longer be correct.
> There are alternatives to Solaar that do not have this issue, like
> libratbag, but these generally are feature lacking on the productivity line
> of Logitech projects.
>
> Sorry I wasn't able to help much, but I hope that this clarifies things a
> bit, and helps you solve your problem.
>
Thanks. You are right, it works with solaar uninstalled, I only installed it
to check the details of a device not recognized by the kernel.
So the patch as send to you before is then upstreamable. Is there anything
more I need to do, to facilitate the upstreaming?
Best regards
Allan
^ permalink raw reply
* Re: hid-logitech-dj support for Anywhere 3SB
From: Filipe Laíns @ 2024-04-13 8:33 UTC (permalink / raw)
To: Allan Sandfeld Jensen, linux-input
In-Reply-To: <6038382.lOV4Wx5bFT@twilight>
[-- Attachment #1: Type: text/plain, Size: 4194 bytes --]
On Sat, 2024-04-13 at 10:20 +0200, Allan Sandfeld Jensen wrote:
> On Friday 12 April 2024 21:53:31 CEST Filipe Laíns wrote:
> > On Fri, 2024-04-12 at 11:10 +0200, Allan Sandfeld Jensen wrote:
> > > Hello,
> > >
> > > I am writing because you are listed as author of the hid-logitech-dj
> > > driver. I recently bought a Logitech Anywhere 3SB mouse, and found Linux
> > > didn't recognize it. Thinking it was a simple case of new IDs not
> > > recognized, I quickly added them to the logitch HID++ drivers (patch
> > > attached), both for USB with the new receiver and for the Bluetooth
> > > direct connection.
> > >
> > > I have noticed however that the patch while causing them to be recognized
> > > and interacted with as HID++ devices, it has a flaw. The scroll wheel
> > > events are reported by the linux kernel as being in hires mode, while
> > > haven't actually enabled it on the mouse. You can fix that using Solaar,
> > > but some piece is missing to enable it correctly in the driver. Since
> > > this is no longer a trivial fix. I wanted to reach out. Do you have any
> > > suggestions?
> > >
> > > Best regards
> > > Allan
> >
> > Hi Allan,
> >
> > Thank you for reaching out.
> >
> > What likely is happening here is Solaar overwriting the configuration that
> > the kernel driver sets, as that would happen after the driver talks to the
> > device.
> >
> > The settings in question need support in both the kernel and the userspace
> > input stack (libinput) for them to work appropriately, it's not like
> > configuring RGB or other sort setting on the device that works standalone.
> We already have the support in the kernel and libinput. That is why I am
> expanding it to recognize this new device id.(?)
Yes, that sounds right.
> > I have, multiple times now, asked for Solaar to not expose these low level
> > settings that need support from other parts of the input stack, leaving them
> > to the kernel to configure.
> > I have been inactive in the Solaar project for quite some time now, so I
> > don't feel like yet again make a big deal out of this there, so that this
> > decision is reconsidered. I have already spent a significant amount of
> > effort there, and nowadays I barely have energy to go through my day and
> > deal with my all my responsibilities and other OSS project involvements, so
> > I sadly have no more energy to spare there.
> >
> > My recommendation is: disable Solaar from running at startup, restart the
> > system, and see if that solves your problem. If it does, report this issue
> > again to the Solaar upstream, then depending on that outcome, make a
> > decision on how to proceed. It may be that setting the high-resolution
> > settings in Solaar, which are expected by the driver, works, but it might
> > not be super reliable, because since Solaar is overwriting the settings
> > configured by the kernel driver, if anything in the kernel driver changes,
> > then the setting you have configured in Solaar might no longer be correct.
> > There are alternatives to Solaar that do not have this issue, like
> > libratbag, but these generally are feature lacking on the productivity line
> > of Logitech projects.
> >
> > Sorry I wasn't able to help much, but I hope that this clarifies things a
> > bit, and helps you solve your problem.
> >
> Thanks. You are right, it works with solaar uninstalled, I only installed it
> to check the details of a device not recognized by the kernel.
>
> So the patch as send to you before is then upstreamable. Is there anything
> more I need to do, to facilitate the upstreaming?
>
> Best regards
> Allan
Great to hear!
The patch you sent seems pretty good for upstreaming, I would maybe just split
the Makefile changes into a separate patch and submit those separately, if
that's something you actually want to upstream. Additionally, it seems to me
like the mouse can work wired, so I would also add the USB PID of the wired
connection to the hidpp driver, that way everything should work as expected on
all interfaces.
Let me know if you need anything else.
Cheers,
Filipe Laíns
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v9 00/38] ep93xx device tree conversion
From: Uwe Kleine-König @ 2024-04-13 8:48 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Nikita Shubin, Hartley Sweeten, Alexander Sverdlin, Russell King,
Lukasz Majewski, Linus Walleij, Bartosz Golaszewski,
Andy Shevchenko, Michael Turquette, Stephen Boyd,
Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Vinod Koul, Wim Van Sebroeck, Guenter Roeck, Thierry Reding,
Mark Brown, David S . Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Damien Le Moal, Sergey Shtylyov,
Dmitry Torokhov, Liam Girdwood, Jaroslav Kysela, Takashi Iwai,
Ralf Baechle, Aaron Wu, Lee Jones, Olof Johansson, Niklas Cassel,
linux-arm-kernel, linux-kernel, open list:GPIO SUBSYSTEM,
linux-clk, linux-pm, devicetree, dmaengine, linux-watchdog,
linux-pwm, linux-spi, Netdev, linux-mtd, linux-ide, linux-input,
linux-sound, Bartosz Golaszewski, Krzysztof Kozlowski,
Andy Shevchenko, Andrew Lunn, Andy Shevchenko
In-Reply-To: <66e1da99-5cf4-4506-b0bf-4bdf04959f41@app.fastmail.com>
[-- Attachment #1: Type: text/plain, Size: 2501 bytes --]
Hello Arnd,
On Tue, Mar 26, 2024 at 11:07:06AM +0100, Arnd Bergmann wrote:
> On Tue, Mar 26, 2024, at 10:18, Nikita Shubin via B4 Relay wrote:
> > The goal is to recieve ACKs for all patches in series to merge it via
> > Arnd branch.
>
> Thank you for the continued updates, I really hope we can merge
> it all for 6.10. I've looked through it again and I'm pretty much
> ready to just merge it, though I admit that the process is not
> working out that great, and it would probably have been quicker
> to add DT support to drivers individually through the subsystem
> trees.
>
> > Stephen Boyd, Vinod Koul PLEASE! give some comments on following, couse
> > i hadn't one for a couple of iterations already:
> >
> > Following patches require attention from Stephen Boyd, as they were
> > converted to aux_dev as suggested:
> >
> > - ARM: ep93xx: add regmap aux_dev
> > - clk: ep93xx: add DT support for Cirrus EP93xx
> >
> > Following patches require attention from Vinod Koul:
> >
> > - dma: cirrus: Convert to DT for Cirrus EP93xx
> > - dma: cirrus: remove platform code
>
> I suspect that Stephen and Vinod may be missing this, as reviewing
> a 38 patch series tends to be a lot of work, and they may have
> missed that they are on the critical path here. I certainly
> tend to just ignore an entire thread when it looks like I'm not
> immediately going to be reviewing it all and other people are
> likely to have more comments first, so I'm not blaming them.
>
> To better catch their attention, I would suggest you repost the
> two smaller sets of patches as a separate series, with only the
> relevant people on Cc. Please also include the respective
> bindings when you send send these patches to Stephen and
> Vinod.
It seems this happend for the clock series; it's at
https://lore.kernel.org/all/20240408-ep93xx-clk-v1-0-1d0f4c324647@maquefel.me/
and received an ack by Stephen.
Vinod gave some feedback in this thread with some remarks that need
addressing.
With the latter I wonder if the plan to get this as a whole into v6.10
is screwed and if I should pick up the PWM bits (patches #12, #13 and
maybe #38) via my tree. Patch #38 touches arch/arm and
include/linux/soc, so I wouldn't pick that one up without an explicit
ack by (I guess) Arnd.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: hid-logitech-dj support for Anywhere 3SB
From: Allan Sandfeld Jensen @ 2024-04-13 8:52 UTC (permalink / raw)
To: linux-input, Filipe Laíns
In-Reply-To: <7f829b15729acba79d24299da0c12cbfead175c5.camel@riseup.net>
[-- Attachment #1: Type: text/plain, Size: 5324 bytes --]
On Saturday 13 April 2024 10:33:29 CEST Filipe Laíns wrote:
> On Sat, 2024-04-13 at 10:20 +0200, Allan Sandfeld Jensen wrote:
> > On Friday 12 April 2024 21:53:31 CEST Filipe Laíns wrote:
> > > On Fri, 2024-04-12 at 11:10 +0200, Allan Sandfeld Jensen wrote:
> > > > Hello,
> > > >
> > > > I am writing because you are listed as author of the hid-logitech-dj
> > > > driver. I recently bought a Logitech Anywhere 3SB mouse, and found
> > > > Linux
> > > > didn't recognize it. Thinking it was a simple case of new IDs not
> > > > recognized, I quickly added them to the logitch HID++ drivers (patch
> > > > attached), both for USB with the new receiver and for the Bluetooth
> > > > direct connection.
> > > >
> > > > I have noticed however that the patch while causing them to be
> > > > recognized
> > > > and interacted with as HID++ devices, it has a flaw. The scroll wheel
> > > > events are reported by the linux kernel as being in hires mode, while
> > > > haven't actually enabled it on the mouse. You can fix that using
> > > > Solaar,
> > > > but some piece is missing to enable it correctly in the driver. Since
> > > > this is no longer a trivial fix. I wanted to reach out. Do you have
> > > > any
> > > > suggestions?
> > > >
> > > > Best regards
> > > > Allan
> > >
> > > Hi Allan,
> > >
> > > Thank you for reaching out.
> > >
> > > What likely is happening here is Solaar overwriting the configuration
> > > that
> > > the kernel driver sets, as that would happen after the driver talks to
> > > the
> > > device.
> > >
> > > The settings in question need support in both the kernel and the
> > > userspace
> > > input stack (libinput) for them to work appropriately, it's not like
> > > configuring RGB or other sort setting on the device that works
> > > standalone.
> >
> > We already have the support in the kernel and libinput. That is why I am
> > expanding it to recognize this new device id.(?)
>
> Yes, that sounds right.
>
> > > I have, multiple times now, asked for Solaar to not expose these low
> > > level
> > > settings that need support from other parts of the input stack, leaving
> > > them to the kernel to configure.
> > > I have been inactive in the Solaar project for quite some time now, so I
> > > don't feel like yet again make a big deal out of this there, so that
> > > this
> > > decision is reconsidered. I have already spent a significant amount of
> > > effort there, and nowadays I barely have energy to go through my day and
> > > deal with my all my responsibilities and other OSS project involvements,
> > > so
> > > I sadly have no more energy to spare there.
> > >
> > > My recommendation is: disable Solaar from running at startup, restart
> > > the
> > > system, and see if that solves your problem. If it does, report this
> > > issue
> > > again to the Solaar upstream, then depending on that outcome, make a
> > > decision on how to proceed. It may be that setting the high-resolution
> > > settings in Solaar, which are expected by the driver, works, but it
> > > might
> > > not be super reliable, because since Solaar is overwriting the settings
> > > configured by the kernel driver, if anything in the kernel driver
> > > changes,
> > > then the setting you have configured in Solaar might no longer be
> > > correct.
> > > There are alternatives to Solaar that do not have this issue, like
> > > libratbag, but these generally are feature lacking on the productivity
> > > line
> > > of Logitech projects.
> > >
> > > Sorry I wasn't able to help much, but I hope that this clarifies things
> > > a
> > > bit, and helps you solve your problem.
> >
> > Thanks. You are right, it works with solaar uninstalled, I only installed
> > it to check the details of a device not recognized by the kernel.
> >
> > So the patch as send to you before is then upstreamable. Is there anything
> > more I need to do, to facilitate the upstreaming?
> >
> > Best regards
> > Allan
>
> Great to hear!
>
> The patch you sent seems pretty good for upstreaming, I would maybe just
> split the Makefile changes into a separate patch and submit those
> separately, if that's something you actually want to upstream.
> Additionally, it seems to me like the mouse can work wired, so I would also
> add the USB PID of the wired connection to the hidpp driver, that way
> everything should work as expected on all interfaces.
>
Right. I thought I had remove those changes. I had the weirdest issue when
building the kernel, where echo wouldn't terminate. It worked when I replaced
echo with another command, so I ended up using perl -e print. Still no idea, I
assume some interaction between my shell and the combination of quiet and echo
in a Makefile, but it doesnt matter, not part of the patch.
It doesn't seem like the mouse communicates over the USB cable, only draws
power. This appears consistent with my old Anywhere 2S mouse.
So where should I send the patch now? It has been at least 15 years since I
contributed anything to the kernel, and I understand sending patches to the
central mailing list is frowned upon now. Do you take if from here, or do I
need to send it to a submodule maintainer above you?
Best regards
Allan
[-- Attachment #2: 0002-Logitech-Anywhere-3SB-support.patch --]
[-- Type: text/x-patch, Size: 3678 bytes --]
From f998faa55c4f5988a958f7af50eacfcb0452451c Mon Sep 17 00:00:00 2001
From: Allan Sandfeld Jensen <allan.jensen@qt.io>
Date: Wed, 10 Apr 2024 15:23:40 +0200
Subject: [PATCH 2/2] Logitech Anywhere 3SB support
---
drivers/hid/hid-ids.h | 1 +
drivers/hid/hid-logitech-dj.c | 10 +++++++++-
drivers/hid/hid-logitech-hidpp.c | 2 ++
3 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 2235d78784b1..4b79c4578d32 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -849,6 +849,7 @@
#define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED_1 0xc539
#define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED_1_1 0xc53f
#define USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_POWERPLAY 0xc53a
+#define USB_DEVICE_ID_LOGITECH_BOLT_RECEIVER 0xc548
#define USB_DEVICE_ID_SPACETRAVELLER 0xc623
#define USB_DEVICE_ID_SPACENAVIGATOR 0xc626
#define USB_DEVICE_ID_DINOVO_DESKTOP 0xc704
diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index c358778e070b..92b41ae5a47c 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -120,6 +120,7 @@ enum recvr_type {
recvr_type_27mhz,
recvr_type_bluetooth,
recvr_type_dinovo,
+ recvr_type_bolt,
};
struct dj_report {
@@ -1068,6 +1069,7 @@ static void logi_hidpp_recv_queue_notif(struct hid_device *hdev,
workitem.reports_supported |= STD_KEYBOARD;
break;
case 0x0f:
+ case 0x10:
case 0x11:
device_type = "eQUAD Lightspeed 1.2";
logi_hidpp_dev_conn_notif_equad(hdev, hidpp_report, &workitem);
@@ -1430,7 +1432,8 @@ static int logi_dj_ll_parse(struct hid_device *hid)
dbg_hid("%s: sending a mouse descriptor, reports_supported: %llx\n",
__func__, djdev->reports_supported);
if (djdev->dj_receiver_dev->type == recvr_type_gaming_hidpp ||
- djdev->dj_receiver_dev->type == recvr_type_mouse_only)
+ djdev->dj_receiver_dev->type == recvr_type_mouse_only ||
+ djdev->dj_receiver_dev->type == recvr_type_bolt)
rdcat(rdesc, &rsize, mse_high_res_descriptor,
sizeof(mse_high_res_descriptor));
else if (djdev->dj_receiver_dev->type == recvr_type_27mhz)
@@ -1773,6 +1776,7 @@ static int logi_dj_probe(struct hid_device *hdev,
case recvr_type_dj: no_dj_interfaces = 3; break;
case recvr_type_hidpp: no_dj_interfaces = 2; break;
case recvr_type_gaming_hidpp: no_dj_interfaces = 3; break;
+ case recvr_type_bolt: no_dj_interfaces = 4; break;
case recvr_type_mouse_only: no_dj_interfaces = 2; break;
case recvr_type_27mhz: no_dj_interfaces = 2; break;
case recvr_type_bluetooth: no_dj_interfaces = 2; break;
@@ -1950,6 +1954,10 @@ static const struct hid_device_id logi_dj_receivers[] = {
HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
USB_DEVICE_ID_LOGITECH_UNIFYING_RECEIVER_2),
.driver_data = recvr_type_dj},
+ { /* Logitech bolt receiver (0xc548) */
+ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
+ USB_DEVICE_ID_LOGITECH_BOLT_RECEIVER),
+ .driver_data = recvr_type_bolt},
{ /* Logitech Nano mouse only receiver (0xc52f) */
HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 3c00e6ac8e76..509142982daa 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -4380,6 +4380,8 @@ static const struct hid_device_id hidpp_devices[] = {
HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb023) },
{ /* MX Master 3S mouse over Bluetooth */
HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb034) },
+ { /* MX Anywhere 3SB mouse over Bluetooth */
+ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb038) },
{}
};
--
2.39.2
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox