* Re: [PATCH v9 4/4] input: pm8xxx-vibrator: add new SPMI vibrator support
From: Dmitry Baryshkov @ 2024-04-11 11:02 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: <20240411-pm8xxx-vibrator-new-design-v9-4-7bf56cb92b28@quicinc.com>
On Thu, 11 Apr 2024 at 11:32, Fenglin Wu via B4 Relay
<devnull+quic_fenglinw.quicinc.com@kernel.org> wrote:
>
> 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 | 51 +++++++++++++++++++++++++++++-------
> 1 file changed, 42 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
> index 2959edca8eb9..35bb6f450fd2 100644
> --- a/drivers/input/misc/pm8xxx-vibrator.c
> +++ b/drivers/input/misc/pm8xxx-vibrator.c
> @@ -12,10 +12,10 @@
> #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,6 +26,9 @@ 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;
> };
>
> @@ -45,6 +48,18 @@ static struct pm8xxx_regs pm8916_regs = {
> .drv_en_manual_mask = 0,
> };
>
> +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,
> +};
> +
> /**
> * struct pm8xxx_vib - structure to hold vibrator data
> * @vib_input_dev: input device supporting force feedback
> @@ -53,6 +68,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 +81,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 +99,10 @@ 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;
>
> + /* vibrator without drv2_addr needs be programmed in step increments */
How are these two items related? Are you using vib->drv2_addr as a
marker for 'particular generation'? In such a case please use a flag
instead.
The rest looks good to me.
> + if (!vib->drv2_addr)
> + vib->level /= VIB_PER_STEP_mV(vib);
> +
> if (on)
> val |= (vib->level << regs->drv_shift) & regs->drv_mask;
> else
> @@ -93,6 +114,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 +147,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);
> @@ -203,6 +234,7 @@ static int pm8xxx_vib_probe(struct platform_device *pdev)
>
> 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);
> @@ -257,6 +289,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
>
>
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH 2/5] mfd: add driver for Marvell 88PM886 PMIC
From: Lee Jones @ 2024-04-11 11:37 UTC (permalink / raw)
To: Karel Balej
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Dmitry Torokhov,
Liam Girdwood, Mark Brown, devicetree, linux-kernel, linux-input,
Duje Mihanović, ~postmarketos/upstreaming, phone-devel
In-Reply-To: <20240331105608.7338-4-balejk@matfyz.cz>
On Sun, 31 Mar 2024, Karel Balej wrote:
> Marvell 88PM886 is a PMIC which provides various functions such as
> onkey, battery, charger and regulators. It is found for instance in the
> samsung,coreprimevelte smartphone with which this was tested. Implement
> basic support to allow for the use of regulators and onkey.
>
> Signed-off-by: Karel Balej <balejk@matfyz.cz>
> ---
>
> Notes:
> v1:
> - Address Mark's feedback:
> - Move regmap config back out of the header and rename it. Also lower
> its maximum register based on what's actually used in the downstream
> code.
> RFC v4:
> - Use MFD_CELL_* macros.
> - Address Lee's feedback:
> - Do not define regmap_config.val_bits and .reg_bits.
> - Drop everything regulator related except mfd_cell (regmap
> initialization, IDs enum etc.). Drop pm886_initialize_subregmaps.
> - Do not store regmap pointers as an array as there is now only one
> regmap. Also drop the corresponding enum.
> - Move regmap_config to the header as it is needed in the regulators
> driver.
> - pm886_chip.whoami -> chip_id
> - Reword chip ID mismatch error message and print the ID as
> hexadecimal.
> - Fix includes in include/linux/88pm886.h.
> - Drop the pm886_irq_number enum and define the (for the moment) only
> IRQ explicitly.
> - Have only one MFD cell for all regulators as they are now registered
> all at once in the regulators driver.
> - Reword commit message.
> - Make device table static and remove comma after the sentinel to signal
> that nothing should come after it.
> RFC v3:
> - Drop onkey cell .of_compatible.
> - Rename LDO page offset and regmap to REGULATORS.
> RFC v2:
> - Remove some abstraction.
> - Sort includes alphabetically and add linux/of.h.
> - Depend on OF, remove of_match_ptr and add MODULE_DEVICE_TABLE.
> - Use more temporaries and break long lines.
> - Do not initialize ret in probe.
> - Use the wakeup-source DT property.
> - Rename ret to err.
> - Address Lee's comments:
> - Drop patched in presets for base regmap and related defines.
> - Use full sentences in comments.
> - Remove IRQ comment.
> - Define regmap_config member values.
> - Rename data to sys_off_data.
> - Add _PMIC suffix to Kconfig.
> - Use dev_err_probe.
> - Do not store irq_data.
> - s/WHOAMI/CHIP_ID
> - Drop LINUX part of include guard name.
> - Merge in the regulator series modifications in order to have more
> devices and modify the commit message accordingly. Changes with
> respect to the original regulator series patches:
> - ret -> err
> - Add temporary for dev in pm88x_initialize_subregmaps.
> - Drop of_compatible for the regulators.
> - Do not duplicate LDO regmap for bucks.
> - Rewrite commit message.
>
> drivers/mfd/88pm886.c | 157 ++++++++++++++++++++++++++++++++++++
> drivers/mfd/Kconfig | 12 +++
> drivers/mfd/Makefile | 1 +
> include/linux/mfd/88pm886.h | 30 +++++++
> 4 files changed, 200 insertions(+)
> create mode 100644 drivers/mfd/88pm886.c
> create mode 100644 include/linux/mfd/88pm886.h
>
> diff --git a/drivers/mfd/88pm886.c b/drivers/mfd/88pm886.c
> new file mode 100644
> index 000000000000..e06d418a5da9
> --- /dev/null
> +++ b/drivers/mfd/88pm886.c
> @@ -0,0 +1,157 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include <linux/i2c.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/notifier.h>
> +#include <linux/of.h>
> +#include <linux/reboot.h>
> +#include <linux/regmap.h>
> +
> +#include <linux/mfd/88pm886.h>
> +
> +#define PM886_REG_INT_STATUS1 0x05
> +
> +#define PM886_REG_INT_ENA_1 0x0a
> +#define PM886_INT_ENA1_ONKEY BIT(0)
> +
> +#define PM886_IRQ_ONKEY 0
> +
> +#define PM886_REGMAP_CONF_MAX_REG 0xef
Why have you split the defines up between here and the header?
Please place them all in the header.
> +static const struct regmap_config pm886_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = PM886_REGMAP_CONF_MAX_REG,
> +};
> +
> +static struct regmap_irq pm886_regmap_irqs[] = {
> + REGMAP_IRQ_REG(PM886_IRQ_ONKEY, 0, PM886_INT_ENA1_ONKEY),
> +};
> +
> +static struct regmap_irq_chip pm886_regmap_irq_chip = {
> + .name = "88pm886",
> + .irqs = pm886_regmap_irqs,
> + .num_irqs = ARRAY_SIZE(pm886_regmap_irqs),
> + .num_regs = 4,
> + .status_base = PM886_REG_INT_STATUS1,
> + .ack_base = PM886_REG_INT_STATUS1,
> + .unmask_base = PM886_REG_INT_ENA_1,
> +};
> +
> +static struct resource pm886_onkey_resources[] = {
> + DEFINE_RES_IRQ_NAMED(PM886_IRQ_ONKEY, "88pm886-onkey"),
> +};
> +
> +static struct mfd_cell pm886_devs[] = {
> + MFD_CELL_RES("88pm886-onkey", pm886_onkey_resources),
> + MFD_CELL_NAME("88pm886-regulator"),
> +};
> +
> +static int pm886_power_off_handler(struct sys_off_data *sys_off_data)
> +{
> + struct pm886_chip *chip = sys_off_data->cb_data;
> + struct regmap *regmap = chip->regmap;
> + struct device *dev = &chip->client->dev;
> + int err;
> +
> + err = regmap_update_bits(regmap, PM886_REG_MISC_CONFIG1, PM886_SW_PDOWN,
> + PM886_SW_PDOWN);
Use 100-chars to avoid the '\n'.
> + if (err) {
> + dev_err(dev, "Failed to power off the device: %d\n", err);
> + return NOTIFY_BAD;
> + }
> + return NOTIFY_DONE;
> +}
> +
> +static int pm886_setup_irq(struct pm886_chip *chip,
> + struct regmap_irq_chip_data **irq_data)
> +{
> + struct regmap *regmap = chip->regmap;
> + struct device *dev = &chip->client->dev;
> + int err;
> +
> + /* Set interrupt clearing mode to clear on write. */
> + err = regmap_update_bits(regmap, PM886_REG_MISC_CONFIG2,
> + PM886_INT_INV | PM886_INT_CLEAR | PM886_INT_MASK_MODE,
> + PM886_INT_WC);
> + if (err) {
> + dev_err(dev, "Failed to set interrupt clearing mode: %d\n", err);
> + return err;
> + }
> +
> + err = devm_regmap_add_irq_chip(dev, regmap, chip->client->irq,
> + IRQF_ONESHOT, -1, &pm886_regmap_irq_chip,
> + irq_data);
> + if (err) {
> + dev_err(dev, "Failed to request IRQ: %d\n", err);
> + return err;
> + }
> +
> + return 0;
> +}
> +
> +static int pm886_probe(struct i2c_client *client)
> +{
> + struct regmap_irq_chip_data *irq_data;
> + struct device *dev = &client->dev;
> + struct pm886_chip *chip;
> + struct regmap *regmap;
> + unsigned int chip_id;
> + int err;
> +
> + chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
> + if (!chip)
> + return -ENOMEM;
> +
> + chip->client = client;
> + chip->chip_id = (uintptr_t)device_get_match_data(dev);
> + i2c_set_clientdata(client, chip);
> +
> + regmap = devm_regmap_init_i2c(client, &pm886_regmap_config);
> + if (IS_ERR(regmap))
> + return dev_err_probe(dev, PTR_ERR(regmap), "Failed to initialize regmap\n");
> + chip->regmap = regmap;
> +
> + err = regmap_read(regmap, PM886_REG_ID, &chip_id);
> + if (err)
> + return dev_err_probe(dev, err, "Failed to read chip ID\n");
> +
> + if (chip->chip_id != chip_id)
> + return dev_err_probe(dev, -EINVAL, "Unsupported chip: 0x%x\n", chip_id);
> +
> + err = pm886_setup_irq(chip, &irq_data);
> + if (err)
> + return err;
> +
> + err = devm_mfd_add_devices(dev, 0, pm886_devs, ARRAY_SIZE(pm886_devs),
Why 0?
> + NULL, 0, regmap_irq_get_domain(irq_data));
> + if (err)
> + return dev_err_probe(dev, err, "Failed to add devices\n");
> +
> + err = devm_register_power_off_handler(dev, pm886_power_off_handler, chip);
> + if (err)
> + return dev_err_probe(dev, err, "Failed to register power off handler\n");
> +
> + device_init_wakeup(dev, device_property_read_bool(dev, "wakeup-source"));
> +
> + return 0;
> +}
> +
> +static const struct of_device_id pm886_of_match[] = {
> + { .compatible = "marvell,88pm886-a1", .data = (void *)PM886_A1_CHIP_ID },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, pm886_of_match);
> +
> +static struct i2c_driver pm886_i2c_driver = {
> + .driver = {
> + .name = "88pm886",
> + .of_match_table = pm886_of_match,
> + },
> + .probe = pm886_probe,
> +};
> +module_i2c_driver(pm886_i2c_driver);
> +
> +MODULE_DESCRIPTION("Marvell 88PM886 PMIC driver");
> +MODULE_AUTHOR("Karel Balej <balejk@matfyz.cz>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 4b023ee229cf..d6a762b2bd47 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -794,6 +794,18 @@ config MFD_88PM860X
> select individual components like voltage regulators, RTC and
> battery-charger under the corresponding menus.
>
> +config MFD_88PM886_PMIC
> + bool "Marvell 88PM886 PMIC"
> + depends on I2C=y
> + depends on OF
> + select REGMAP_I2C
> + select REGMAP_IRQ
> + select MFD_CORE
> + help
> + This enables support for Marvell 88PM886 Power Management IC.
> + This includes the I2C driver and the core APIs _only_, you have to
> + select individual components like onkey under the corresponding menus.
> +
> config MFD_MAX14577
> tristate "Maxim Semiconductor MAX14577/77836 MUIC + Charger Support"
> depends on I2C
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index c66f07edcd0e..d016b7fed354 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -7,6 +7,7 @@
> obj-$(CONFIG_MFD_88PM860X) += 88pm860x.o
> obj-$(CONFIG_MFD_88PM800) += 88pm800.o 88pm80x.o
> obj-$(CONFIG_MFD_88PM805) += 88pm805.o 88pm80x.o
> +obj-$(CONFIG_MFD_88PM886_PMIC) += 88pm886.o
> obj-$(CONFIG_MFD_ACT8945A) += act8945a.o
> obj-$(CONFIG_MFD_SM501) += sm501.o
> obj-$(CONFIG_ARCH_BCM2835) += bcm2835-pm.o
> diff --git a/include/linux/mfd/88pm886.h b/include/linux/mfd/88pm886.h
> new file mode 100644
> index 000000000000..5ce30a3b85aa
> --- /dev/null
> +++ b/include/linux/mfd/88pm886.h
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __MFD_88PM886_H
> +#define __MFD_88PM886_H
> +
> +#include <linux/i2c.h>
> +#include <linux/regmap.h>
> +
> +#define PM886_A1_CHIP_ID 0xa1
> +
> +#define PM886_REG_ID 0x00
> +
> +#define PM886_REG_STATUS1 0x01
> +#define PM886_ONKEY_STS1 BIT(0)
> +
> +#define PM886_REG_MISC_CONFIG1 0x14
> +#define PM886_SW_PDOWN BIT(5)
> +
> +#define PM886_REG_MISC_CONFIG2 0x15
> +#define PM886_INT_INV BIT(0)
> +#define PM886_INT_CLEAR BIT(1)
> +#define PM886_INT_RC 0x00
> +#define PM886_INT_WC BIT(1)
> +#define PM886_INT_MASK_MODE BIT(2)
> +
> +struct pm886_chip {
> + struct i2c_client *client;
> + unsigned int chip_id;
> + struct regmap *regmap;
> +};
> +#endif /* __MFD_88PM886_H */
> --
> 2.44.0
>
--
Lee Jones [李琼斯]
^ permalink raw reply
* Re: (subset) [PATCH v3] dt-bindings: mfd: twl: Convert trivial subdevices to json-schema
From: Lee Jones @ 2024-04-11 11:39 UTC (permalink / raw)
To: dmitry.torokhov, robh, krzysztof.kozlowski+dt, conor+dt, lee,
alexandre.belloni, wim, linux, linux-input, devicetree,
linux-kernel, linux-rtc, linux-watchdog, sre, Andreas Kemnade
In-Reply-To: <20240401081831.456828-1-andreas@kemnade.info>
On Mon, 01 Apr 2024 10:18:31 +0200, Andreas Kemnade wrote:
> Convert subdevices with just an interrupt and compatbile to
> json-schema and wire up already converted subdevices.
> RTC is available in all variants, so allow it unconditionally.
> GPADC binding for TWL603X uses two different compatibles, so
> specify just the compatible and do not include it.
>
>
> [...]
Applied, thanks!
[1/1] dt-bindings: mfd: twl: Convert trivial subdevices to json-schema
commit: f9463e7b73bbd3b6dee77c0387e2ebc9e61fe0e4
--
Lee Jones [李琼斯]
^ permalink raw reply
* [hid:for-6.10/hid-bpf 5/7] ld.lld: error: undefined symbol: hid_input_report
From: kernel test robot @ 2024-04-11 11:43 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: llvm, oe-kbuild-all, linux-input
tree: https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git for-6.10/hid-bpf
head: 685dadafbde29dc3d6b7a13be284d684b06d4d4f
commit: 9be50ac30a83896a753ab9f64e941763bb7900be [5/7] HID: bpf: allow to inject HID event from BPF
config: i386-buildonly-randconfig-001-20240411 (https://download.01.org/0day-ci/archive/20240411/202404111903.69WRBBjD-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240411/202404111903.69WRBBjD-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/202404111903.69WRBBjD-lkp@intel.com/
All errors (new ones prefixed by >>):
>> ld.lld: error: undefined symbol: hid_input_report
>>> referenced by hid_bpf_dispatch.c
>>> drivers/hid/bpf/hid_bpf_dispatch.o:(hid_bpf_input_report) in archive vmlinux.a
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH 1/5] dt-bindings: mfd: add entry for Marvell 88PM886 PMIC
From: Krzysztof Kozlowski @ 2024-04-11 12:08 UTC (permalink / raw)
To: Karel Balej, Lee Jones, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Dmitry Torokhov, Liam Girdwood, Mark Brown,
devicetree, linux-kernel, linux-input
Cc: Duje Mihanović, ~postmarketos/upstreaming, phone-devel
In-Reply-To: <20240331105608.7338-3-balejk@matfyz.cz>
On 31/03/2024 12:46, Karel Balej wrote:
> Marvell 88PM886 is a PMIC with several subdevices such as onkey,
> regulators or battery and charger. It comes in at least two revisions,
> A0 and A1 -- only A1 is described here at the moment.
>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Karel Balej <balejk@matfyz.cz>
> ---
>
> Notes:
> RFC v4:
> - Address Krzysztof's comments:
> - Fix regulators indentation.
> - Add Krzysztof's trailer.
So you have four versions and suddenly it became v1? No, keep proper
versioning. This is v5. RFC is not a version, but state of patchset that
it is not ready.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH 1/4] Input: Add trackpoint doubletap and system debug info keycodes
From: Hans de Goede @ 2024-04-11 12:30 UTC (permalink / raw)
To: Dmitry Torokhov, Mark Pearson
Cc: Peter Hutterer, Ilpo Järvinen, Henrique de Moraes Holschuh,
ibm-acpi-devel, platform-driver-x86@vger.kernel.org, linux-input,
linux-kernel, Nitin Joshi1, Vishnu Sankar
In-Reply-To: <ZhcogDESvZmUPEEf@google.com>
Hi Dmitry,
On 4/11/24 2:02 AM, Dmitry Torokhov wrote:
> On Tue, Apr 09, 2024 at 10:17:05PM -0400, Mark Pearson wrote:
>> Hi Dmitry
>>
>> On Tue, Apr 9, 2024, at 9:20 PM, Dmitry Torokhov wrote:
>>> On Tue, Apr 09, 2024 at 02:47:05PM -0700, Dmitry Torokhov wrote:
>>>> On Tue, Apr 09, 2024 at 03:23:52PM +1000, Peter Hutterer wrote:
>>>>> On 09/04/2024 09:31, Dmitry Torokhov wrote:
>>>>>> Hi Mark,
>>>>>>
>>>>>> On Sun, Mar 24, 2024 at 05:07:58PM -0400, Mark Pearson wrote:
>>>>>>> Add support for new input events on Lenovo laptops that need exporting to
>>>>>>> user space.
>>>>>>>
>>>>>>> Lenovo trackpoints are adding the ability to generate a doubletap event.
>>>>>>> Add a new keycode to allow this to be used by userspace.
>>>>>>
>>>>>> What is the intended meaning of this keycode? How does it differ from
>>>>>> the driver sending BTN_LEFT press/release twice?
>>>>>>>
>>>>>>> Lenovo support is using FN+N with Windows to collect needed details for
>>>>>>> support cases. Add a keycode so that we'll be able to provide similar
>>>>>>> support on Linux.
>>>>>>
>>>>>> Is there a userspace consumer for this?
>>>>>
>>>>> Funnily enough XKB has had a keysym for this for decades but it's not
>>>>> hooked up anywhere due to the way it's pointer keys accessibility
>>>>> feature was implemented. Theory is that most of userspace just needs
>>>>> to patch the various pieces together for the new evdev code + keysym,
>>>>> it's not really any different to handling a volume key (except this
>>>>> one needs to be assignable).
>>>>
>>>> What is the keysym? If we can make them relatable to each other that
>>>> would be good. Or maybe we could find a matching usage from HID usage
>>>> tables...
>>>
>>> I was looking through the existing codes and I see:
>>>
>>> #define KEY_INFO 0x166 /* AL OEM Features/Tips/Tutorial */
>>>
>>> We also have KEY_VENDOR used in a few drivers/plafrom/x86, including
>>> thinkkpad_acpi.c and I wonder if it would be suitable for this vendor
>>> specific debug info collection application (which I honestly doubt will
>>> materialize).
>>>
>>
>> That's a somewhat disappointing note on your doubts, is that based on
>> anything? Just wondering what we've done to deserve that criticism.
>
> Sorry, this was not meant as a criticism really, but you mentioned
> yourself that there isn't anything in the works yet, you just have some
> plans.
>
> For such a project to succeed Lenovo needs to invest into selling
> devices with Linux as a primary operating system, and it has to be
> consumer segment (or small business, because for corporate they
> typically roll their own support channels). The case of retrofitting
> Linux onto a that device originally came with Windows OS rarely gets
> much if any response from the normal support channels.
>
> Is this something that is actually happening?
Yes, Lenovo is actually offering Fedora as an OS choice when
ordering ThinkPads directly from their website in many countries
including when ordering as a consumer.
And unlike other vendor's Linux preloads which often use a kernel
with downstream laptop specific changes these laptops are running
unmodified Fedora kernels, which themselves are almost pristine
upstream kernels.
Lenovo (Mark) has been really good the last couple of years in
making sure that their hw just works with mainline kernels without
any downstream vendor specific patches.
>> That aside, I guess KEY_INFO or KEY_VENDOR could be a good fit (I
>> personally don't think KEY_CONFIG matches well), but I would be
>> worried about clashing with existing functionality.
Using KEY_INFO / KEY_VENDOR works for me too. So maybe we should
just go with one of those 2 ?
Regards,
Hans
^ permalink raw reply
* Re: [PATCH v9 2/4] input: pm8xxx-vibrator: refactor to support new SPMI vibrator
From: Fenglin Wu @ 2024-04-11 13:43 UTC (permalink / raw)
To: Dmitry Baryshkov
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: <CAA8EJpoL9vCAUgWmHcoxppo_gJqaw_xqdYqcJkS6Xza-5aSh3A@mail.gmail.com>
On 2024/4/11 18:58, Dmitry Baryshkov wrote:
> On Thu, 11 Apr 2024 at 11:32, Fenglin Wu via B4 Relay
> <devnull+quic_fenglinw.quicinc.com@kernel.org> wrote:
>>
>> 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 | 42 ++++++++++++++++++++++++------------
>> 1 file changed, 28 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
>> index 89f0f1c810d8..2959edca8eb9 100644
>> --- a/drivers/input/misc/pm8xxx-vibrator.c
>> +++ b/drivers/input/misc/pm8xxx-vibrator.c
>> @@ -20,26 +20,26 @@
>> #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 = 0x4A,
>
> If the DT already has reg = <0x4a> and you add drv_offset = 0x4a,
> which register will be used by the driver?
>
> Also, while we are at it, please downcase all the hex numbers that you
> are touching.
>
For SSBI vibrator, the "reg" value defined in DT is not used, see below.
>> .drv_mask = 0xf8,
>> .drv_shift = 3,
>> .drv_en_manual_mask = 0xfc,
>> };
>>
>> static struct pm8xxx_regs pm8916_regs = {
>> - .enable_addr = 0xc046,
>> + .enable_offset = 0x46,
[...]
>> @@ -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);
>> @@ -190,13 +193,24 @@ static int pm8xxx_vib_probe(struct platform_device *pdev)
>>
>> regs = of_device_get_match_data(&pdev->dev);
>>
>> + if (regs->enable_offset != 0) {
>> + 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;
>> + }
>> + }
>> +
>> + vib->enable_addr = reg_base + regs->enable_offset;
>> + vib->drv_addr = reg_base + regs->drv_offset;
The reg_base is initialized as 0 and it is assigned as the "reg" value
defined in DT only for SPMI vibrators.
>> +
>> /* 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
* Re: [PATCH v9 4/4] input: pm8xxx-vibrator: add new SPMI vibrator support
From: Fenglin Wu @ 2024-04-11 13:51 UTC (permalink / raw)
To: Dmitry Baryshkov
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: <CAA8EJprJ4s-o1uPiPjRpq4nwG4cdV7K8XMhVLOQn2D=kJLiVzQ@mail.gmail.com>
On 2024/4/11 19:02, Dmitry Baryshkov wrote:
> On Thu, 11 Apr 2024 at 11:32, Fenglin Wu via B4 Relay
> <devnull+quic_fenglinw.quicinc.com@kernel.org> wrote:
>>
>> 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 | 51 +++++++++++++++++++++++++++++-------
>> 1 file changed, 42 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
>> index 2959edca8eb9..35bb6f450fd2 100644
>> --- a/drivers/input/misc/pm8xxx-vibrator.c
>> +++ b/drivers/input/misc/pm8xxx-vibrator.c
>> @@ -12,10 +12,10 @@
>> #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,6 +26,9 @@ 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;
>> };
>>
>> @@ -45,6 +48,18 @@ static struct pm8xxx_regs pm8916_regs = {
>> .drv_en_manual_mask = 0,
>> };
>>
>> +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,
>> +};
>> +
>> /**
>> * struct pm8xxx_vib - structure to hold vibrator data
>> * @vib_input_dev: input device supporting force feedback
>> @@ -53,6 +68,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 +81,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 +99,10 @@ 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;
>>
>> + /* vibrator without drv2_addr needs be programmed in step increments */
>
> How are these two items related? Are you using vib->drv2_addr as a
> marker for 'particular generation'? In such a case please use a flag
> instead.
>
> The rest looks good to me.
>
Are you suggesting to add a flag in pm8xxx_vib as a discriminator for
the new generation? I actually tried to avoid that because of this comment:
https://lore.kernel.org/linux-arm-msm/ZgXSBiQcBEbwF060@google.com/#t
>> + if (!vib->drv2_addr)
>> + vib->level /= VIB_PER_STEP_mV(vib);
>> +
>> if (on)
>> val |= (vib->level << regs->drv_shift) & regs->drv_mask;
>> else
>> @@ -93,6 +114,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 +147,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);
>> @@ -203,6 +234,7 @@ static int pm8xxx_vib_probe(struct platform_device *pdev)
>>
>> 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);
>> @@ -257,6 +289,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
* Re: [PATCH] HID: bpf: fix hid_bpf_input_report() when hid-core is not ready
From: Benjamin Tissoires @ 2024-04-11 14:05 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires; +Cc: linux-input, linux-kernel
In-Reply-To: <20240411-fix-hid-bpf-v1-1-4ae913031a8c@kernel.org>
On Thu, 11 Apr 2024 09:05:56 +0200, Benjamin Tissoires wrote:
> Reported by linux-next:
> After merging the hid tree, today's linux-next build (x86_64 allmodconfig)
> failed like this:
>
> x86_64-linux-gnu-ld: vmlinux.o: in function `hid_bpf_input_report':
> (.text+0x1c75181): undefined reference to `hid_input_report'
>
> [...]
Applied to hid/hid.git (for-6.10/hid-bpf), thanks!
[1/1] HID: bpf: fix hid_bpf_input_report() when hid-core is not ready
https://git.kernel.org/hid/hid/c/b912cf042072
Cheers,
--
Benjamin Tissoires <bentiss@kernel.org>
^ permalink raw reply
* Re: [PATCH v9 2/4] input: pm8xxx-vibrator: refactor to support new SPMI vibrator
From: Dmitry Baryshkov @ 2024-04-11 14:05 UTC (permalink / raw)
To: Fenglin Wu
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: <fa6c8b30-11f3-bd80-67cb-713e4348eccf@quicinc.com>
On Thu, 11 Apr 2024 at 16:45, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
>
>
>
> On 2024/4/11 18:58, Dmitry Baryshkov wrote:
> > On Thu, 11 Apr 2024 at 11:32, Fenglin Wu via B4 Relay
> > <devnull+quic_fenglinw.quicinc.com@kernel.org> wrote:
> >>
> >> 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 | 42 ++++++++++++++++++++++++------------
> >> 1 file changed, 28 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
> >> index 89f0f1c810d8..2959edca8eb9 100644
> >> --- a/drivers/input/misc/pm8xxx-vibrator.c
> >> +++ b/drivers/input/misc/pm8xxx-vibrator.c
> >> @@ -20,26 +20,26 @@
> >> #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 = 0x4A,
> >
> > If the DT already has reg = <0x4a> and you add drv_offset = 0x4a,
> > which register will be used by the driver?
> >
> > Also, while we are at it, please downcase all the hex numbers that you
> > are touching.
> >
> For SSBI vibrator, the "reg" value defined in DT is not used, see below.
>
>
> >> .drv_mask = 0xf8,
> >> .drv_shift = 3,
> >> .drv_en_manual_mask = 0xfc,
> >> };
> >>
> >> static struct pm8xxx_regs pm8916_regs = {
> >> - .enable_addr = 0xc046,
> >> + .enable_offset = 0x46,
>
> [...]
>
> >> @@ -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);
> >> @@ -190,13 +193,24 @@ static int pm8xxx_vib_probe(struct platform_device *pdev)
> >>
> >> regs = of_device_get_match_data(&pdev->dev);
> >>
> >> + if (regs->enable_offset != 0) {
> >> + 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;
> >> + }
> >> + }
> >> +
> >> + vib->enable_addr = reg_base + regs->enable_offset;
> >> + vib->drv_addr = reg_base + regs->drv_offset;
>
> The reg_base is initialized as 0 and it is assigned as the "reg" value
> defined in DT only for SPMI vibrators.
Please don't. This is counterintuitive. We have reg in DT. We should
be using it.
>
> >> +
> >> /* 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
> >>
> >>
> >
> >
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH v9 4/4] input: pm8xxx-vibrator: add new SPMI vibrator support
From: Dmitry Baryshkov @ 2024-04-11 14:21 UTC (permalink / raw)
To: Fenglin Wu
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: <c2ee9ab0-ecb2-aba2-2cc9-653f74d27396@quicinc.com>
On Thu, 11 Apr 2024 at 16:51, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
>
>
>
> On 2024/4/11 19:02, Dmitry Baryshkov wrote:
> > On Thu, 11 Apr 2024 at 11:32, Fenglin Wu via B4 Relay
> > <devnull+quic_fenglinw.quicinc.com@kernel.org> wrote:
> >>
> >> 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 | 51 +++++++++++++++++++++++++++++-------
> >> 1 file changed, 42 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
> >> index 2959edca8eb9..35bb6f450fd2 100644
> >> --- a/drivers/input/misc/pm8xxx-vibrator.c
> >> +++ b/drivers/input/misc/pm8xxx-vibrator.c
> >> @@ -12,10 +12,10 @@
> >> #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,6 +26,9 @@ 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;
> >> };
> >>
> >> @@ -45,6 +48,18 @@ static struct pm8xxx_regs pm8916_regs = {
> >> .drv_en_manual_mask = 0,
> >> };
> >>
> >> +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,
> >> +};
> >> +
> >> /**
> >> * struct pm8xxx_vib - structure to hold vibrator data
> >> * @vib_input_dev: input device supporting force feedback
> >> @@ -53,6 +68,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 +81,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 +99,10 @@ 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;
> >>
> >> + /* vibrator without drv2_addr needs be programmed in step increments */
> >
> > How are these two items related? Are you using vib->drv2_addr as a
> > marker for 'particular generation'? In such a case please use a flag
> > instead.
> >
> > The rest looks good to me.
> >
> Are you suggesting to add a flag in pm8xxx_vib as a discriminator for
> the new generation? I actually tried to avoid that because of this comment:
> https://lore.kernel.org/linux-arm-msm/ZgXSBiQcBEbwF060@google.com/#t
Add a flag for level being programmed in steps or in mV. Using
drv2_addr instead of such flag is a hack.
>
> >> + if (!vib->drv2_addr)
> >> + vib->level /= VIB_PER_STEP_mV(vib);
> >> +
> >> if (on)
> >> val |= (vib->level << regs->drv_shift) & regs->drv_mask;
> >> else
> >> @@ -93,6 +114,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 +147,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);
> >> @@ -203,6 +234,7 @@ static int pm8xxx_vib_probe(struct platform_device *pdev)
> >>
> >> 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);
> >> @@ -257,6 +289,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
> >>
> >>
> >
> >
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH v2] HID: i2c-hid: wait for i2c touchpad deep-sleep to power-up transition
From: Łukasz Majczak @ 2024-04-11 14:23 UTC (permalink / raw)
To: Johan Hovold
Cc: Jiri Kosina, Dmitry Torokhov, Benjamin Tissoires,
Douglas Anderson, Hans de Goede, Maxime Ripard, Kai-Heng Feng,
Johan Hovold, linux-input, linux-kernel, Radoslaw Biernacki
In-Reply-To: <ZhVix-HJrqQbiPrB@hovoldconsulting.com>
> Sure, but what about other transactions that are initiated by the host
> (e.g. SET_POWER)?
>
Somehow it is problematic only on reboot and works just fine on
suspend/resume and
set_power.
I will dig more and try to find out what the difference is.
> Perhaps this hack at probe is enough for your use case, but is an
> incomplete hack and at a minimum you'd need to add a comment explaining
> why it is there.
>
You mean a comment in the code ?
Lukasz
^ permalink raw reply
* Re: [PATCH 2/5] mfd: add driver for Marvell 88PM886 PMIC
From: Karel Balej @ 2024-04-11 15:09 UTC (permalink / raw)
To: Lee Jones
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Dmitry Torokhov,
Liam Girdwood, Mark Brown, devicetree, linux-kernel, linux-input,
Duje Mihanović, ~postmarketos/upstreaming, phone-devel
In-Reply-To: <20240411113726.GH1980182@google.com>
Lee Jones, 2024-04-11T12:37:26+01:00:
[...]
> > diff --git a/drivers/mfd/88pm886.c b/drivers/mfd/88pm886.c
> > new file mode 100644
> > index 000000000000..e06d418a5da9
> > --- /dev/null
> > +++ b/drivers/mfd/88pm886.c
> > @@ -0,0 +1,157 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +#include <linux/i2c.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/module.h>
> > +#include <linux/notifier.h>
> > +#include <linux/of.h>
> > +#include <linux/reboot.h>
> > +#include <linux/regmap.h>
> > +
> > +#include <linux/mfd/88pm886.h>
> > +
> > +#define PM886_REG_INT_STATUS1 0x05
> > +
> > +#define PM886_REG_INT_ENA_1 0x0a
> > +#define PM886_INT_ENA1_ONKEY BIT(0)
> > +
> > +#define PM886_IRQ_ONKEY 0
> > +
> > +#define PM886_REGMAP_CONF_MAX_REG 0xef
>
> Why have you split the defines up between here and the header?
I tried to keep defines tied to the code which uses them and only put
defines needed in multiple places in the header. With the exception of
closely related things, such as register bits which I am keeping
together with the respective register definitions for clarity. Does that
not make sense?
> Please place them all in the header.
Would you then also have me move all the definitions from the regulators
driver there?
[...]
> > + err = devm_mfd_add_devices(dev, 0, pm886_devs, ARRAY_SIZE(pm886_devs),
>
> Why 0?
PLATFORM_DEVID_AUTO then? Or will PLATFORM_DEVID_NONE suffice since the
cells all have different names now (it would probably cause problems
though if the driver was used multiple times for some reason, wouldn't
it?)?
Thank you,
K. B.
^ permalink raw reply
* Re: [PATCH RESEND v10 3/5] mfd: cs40l50: Add support for CS40L50 core driver
From: Lee Jones @ 2024-04-11 15:35 UTC (permalink / raw)
To: James Ogletree
Cc: dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt,
broonie, jeff, patches, linux-sound, linux-input, devicetree
In-Reply-To: <20240408153214.42368-4-jogletre@opensource.cirrus.com>
On Mon, 08 Apr 2024, James Ogletree wrote:
> Introduce support for Cirrus Logic Device CS40L50: a
> haptic driver with waveform memory, integrated DSP,
> and closed-loop algorithms.
>
> The MFD component registers and initializes the device.
>
> Signed-off-by: James Ogletree <jogletre@opensource.cirrus.com>
> ---
> MAINTAINERS | 2 +
> drivers/mfd/Kconfig | 30 ++
> drivers/mfd/Makefile | 4 +
> drivers/mfd/cs40l50-core.c | 570 ++++++++++++++++++++++++++++++++++++
> drivers/mfd/cs40l50-i2c.c | 68 +++++
> drivers/mfd/cs40l50-spi.c | 68 +++++
> include/linux/mfd/cs40l50.h | 137 +++++++++
> 7 files changed, 879 insertions(+)
> create mode 100644 drivers/mfd/cs40l50-core.c
> create mode 100644 drivers/mfd/cs40l50-i2c.c
> create mode 100644 drivers/mfd/cs40l50-spi.c
> create mode 100644 include/linux/mfd/cs40l50.h
Looks like this hasn't been updated since my last review.
--
Lee Jones [李琼斯]
^ permalink raw reply
* Re: [PATCH 2/5] mfd: add driver for Marvell 88PM886 PMIC
From: Lee Jones @ 2024-04-11 16:39 UTC (permalink / raw)
To: Karel Balej
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Dmitry Torokhov,
Liam Girdwood, Mark Brown, devicetree, linux-kernel, linux-input,
Duje Mihanović, ~postmarketos/upstreaming, phone-devel
In-Reply-To: <D0HE07BHD1T8.HQLIBUHTRVGT@matfyz.cz>
On Thu, 11 Apr 2024, Karel Balej wrote:
> Lee Jones, 2024-04-11T12:37:26+01:00:
> [...]
> > > diff --git a/drivers/mfd/88pm886.c b/drivers/mfd/88pm886.c
> > > new file mode 100644
> > > index 000000000000..e06d418a5da9
> > > --- /dev/null
> > > +++ b/drivers/mfd/88pm886.c
> > > @@ -0,0 +1,157 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +#include <linux/i2c.h>
> > > +#include <linux/mfd/core.h>
> > > +#include <linux/module.h>
> > > +#include <linux/notifier.h>
> > > +#include <linux/of.h>
> > > +#include <linux/reboot.h>
> > > +#include <linux/regmap.h>
> > > +
> > > +#include <linux/mfd/88pm886.h>
> > > +
> > > +#define PM886_REG_INT_STATUS1 0x05
> > > +
> > > +#define PM886_REG_INT_ENA_1 0x0a
> > > +#define PM886_INT_ENA1_ONKEY BIT(0)
> > > +
> > > +#define PM886_IRQ_ONKEY 0
> > > +
> > > +#define PM886_REGMAP_CONF_MAX_REG 0xef
> >
> > Why have you split the defines up between here and the header?
>
> I tried to keep defines tied to the code which uses them and only put
> defines needed in multiple places in the header. With the exception of
> closely related things, such as register bits which I am keeping
> together with the respective register definitions for clarity. Does that
> not make sense?
It makes sense and it's a nice thought, but I think it's nicer to keep
them all together, rather than have to worry about which ones are and
which ones are not used here or there. Also, there will be holes in the
definitions, etc.
> > Please place them all in the header.
>
> Would you then also have me move all the definitions from the regulators
> driver there?
I think it would be nice to have them all nice and contiguous.
So, yes.
> [...]
>
> > > + err = devm_mfd_add_devices(dev, 0, pm886_devs, ARRAY_SIZE(pm886_devs),
> >
> > Why 0?
>
> PLATFORM_DEVID_AUTO then? Or will PLATFORM_DEVID_NONE suffice since the
> cells all have different names now (it would probably cause problems
> though if the driver was used multiple times for some reason, wouldn't
> it?)?
You tell me. Please try and understand the code you author. :)
--
Lee Jones [李琼斯]
^ permalink raw reply
* Re: [PATCH v9 2/4] input: pm8xxx-vibrator: refactor to support new SPMI vibrator
From: Fenglin Wu @ 2024-04-12 3:40 UTC (permalink / raw)
To: Dmitry Baryshkov
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: <CAA8EJpqa=5yaTRHuEiYynTDFy53YPFk4R3q_EV8rmsBN1iR5fA@mail.gmail.com>
On 4/11/2024 10:05 PM, Dmitry Baryshkov wrote:
> On Thu, 11 Apr 2024 at 16:45, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
>>
>>
>>
>> On 2024/4/11 18:58, Dmitry Baryshkov wrote:
>>> On Thu, 11 Apr 2024 at 11:32, Fenglin Wu via B4 Relay
>>> <devnull+quic_fenglinw.quicinc.com@kernel.org> wrote:
>>>>
>>>> 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 | 42 ++++++++++++++++++++++++------------
>>>> 1 file changed, 28 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
>>>> index 89f0f1c810d8..2959edca8eb9 100644
>>>> --- a/drivers/input/misc/pm8xxx-vibrator.c
>>>> +++ b/drivers/input/misc/pm8xxx-vibrator.c
>>>> @@ -20,26 +20,26 @@
>>>> #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 = 0x4A,
>>>
>>> If the DT already has reg = <0x4a> and you add drv_offset = 0x4a,
>>> which register will be used by the driver?
>>>
>>> Also, while we are at it, please downcase all the hex numbers that you
>>> are touching.
>>>
>> For SSBI vibrator, the "reg" value defined in DT is not used, see below.
>>
>>
>>>> .drv_mask = 0xf8,
>>>> .drv_shift = 3,
>>>> .drv_en_manual_mask = 0xfc,
>>>> };
>>>>
>>>> static struct pm8xxx_regs pm8916_regs = {
>>>> - .enable_addr = 0xc046,
>>>> + .enable_offset = 0x46,
>>
>> [...]
>>
>>>> @@ -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);
>>>> @@ -190,13 +193,24 @@ static int pm8xxx_vib_probe(struct platform_device *pdev)
>>>>
>>>> regs = of_device_get_match_data(&pdev->dev);
>>>>
>>>> + if (regs->enable_offset != 0) {
>>>> + 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;
>>>> + }
>>>> + }
>>>> +
>>>> + vib->enable_addr = reg_base + regs->enable_offset;
>>>> + vib->drv_addr = reg_base + regs->drv_offset;
>>
>> The reg_base is initialized as 0 and it is assigned as the "reg" value
>> defined in DT only for SPMI vibrators.
>
> Please don't. This is counterintuitive. We have reg in DT. We should
> be using it.
>
Hmm, the original driver doesn't use the reg value defined in DT at all,
Anyway, I can make the SSBI offset to 0, so the base address defined in
the DT will be always added regardless of SSBI or SPMI vibrator. Let me
know.
Thanks
>>
>>>> +
>>>> /* 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
* Re: [PATCH v9 4/4] input: pm8xxx-vibrator: add new SPMI vibrator support
From: Fenglin Wu @ 2024-04-12 4:04 UTC (permalink / raw)
To: Dmitry Baryshkov
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: <CAA8EJppJOQ+-XtgJZa01uqYdqXJdfNznR1OUbWua_myzUqNBUA@mail.gmail.com>
On 4/11/2024 10:21 PM, Dmitry Baryshkov wrote:
> On Thu, 11 Apr 2024 at 16:51, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
>>
>>
>>
>> On 2024/4/11 19:02, Dmitry Baryshkov wrote:
>>> On Thu, 11 Apr 2024 at 11:32, Fenglin Wu via B4 Relay
>>> <devnull+quic_fenglinw.quicinc.com@kernel.org> wrote:
>>>>
>>>> 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 | 51 +++++++++++++++++++++++++++++-------
>>>> 1 file changed, 42 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
>>>> index 2959edca8eb9..35bb6f450fd2 100644
>>>> --- a/drivers/input/misc/pm8xxx-vibrator.c
>>>> +++ b/drivers/input/misc/pm8xxx-vibrator.c
>>>> @@ -12,10 +12,10 @@
>>>> #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,6 +26,9 @@ 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;
>>>> };
>>>>
>>>> @@ -45,6 +48,18 @@ static struct pm8xxx_regs pm8916_regs = {
>>>> .drv_en_manual_mask = 0,
>>>> };
>>>>
>>>> +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,
>>>> +};
>>>> +
>>>> /**
>>>> * struct pm8xxx_vib - structure to hold vibrator data
>>>> * @vib_input_dev: input device supporting force feedback
>>>> @@ -53,6 +68,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 +81,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 +99,10 @@ 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;
>>>>
>>>> + /* vibrator without drv2_addr needs be programmed in step increments */
>>>
>>> How are these two items related? Are you using vib->drv2_addr as a
>>> marker for 'particular generation'? In such a case please use a flag
>>> instead.
>>>
>>> The rest looks good to me.
>>>
>> Are you suggesting to add a flag in pm8xxx_vib as a discriminator for
>> the new generation? I actually tried to avoid that because of this comment:
>> https://lore.kernel.org/linux-arm-msm/ZgXSBiQcBEbwF060@google.com/#t
>
> Add a flag for level being programmed in steps or in mV. Using
> drv2_addr instead of such flag is a hack.
>
Thanks Dmitry.
Does this flag look good to you?
diff --git a/drivers/input/misc/pm8xxx-vibrator.c
b/drivers/input/misc/pm8xxx-vibrator.c
index 35bb6f450fd2..4708f441e5ac 100644
--- a/drivers/input/misc/pm8xxx-vibrator.c
+++ b/drivers/input/misc/pm8xxx-vibrator.c
@@ -30,6 +30,7 @@ struct pm8xxx_regs {
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 = {
@@ -37,6 +38,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 = {
@@ -46,6 +48,7 @@ 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 = {
@@ -58,6 +61,7 @@ static struct pm8xxx_regs pmi632_regs = {
.drv2_mask = GENMASK(3, 0),
.drv2_shift = 8,
.drv_en_manual_mask = 0,
+ .drv_in_step = false,
};
/**
@@ -100,7 +104,7 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib,
bool on)
const struct pm8xxx_regs *regs = vib->regs;
/* vibrator without drv2_addr needs be programmed in step
increments */
- if (!vib->drv2_addr)
+ if (regs->drv_in_step)
vib->level /= VIB_PER_STEP_mV(vib);
if (on)
>>
>>>> + if (!vib->drv2_addr)
>>>> + vib->level /= VIB_PER_STEP_mV(vib);
>>>> +
>>>> if (on)
>>>> val |= (vib->level << regs->drv_shift) & regs->drv_mask;
>>>> else
>>>> @@ -93,6 +114,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 +147,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);
>>>> @@ -203,6 +234,7 @@ static int pm8xxx_vib_probe(struct platform_device *pdev)
>>>>
>>>> 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);
>>>> @@ -257,6 +289,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
* Re: [PATCH v2] HID: i2c-hid: wait for i2c touchpad deep-sleep to power-up transition
From: Kai-Heng Feng @ 2024-04-12 6:28 UTC (permalink / raw)
To: Łukasz Majczak
Cc: Johan Hovold, Jiri Kosina, Dmitry Torokhov, Benjamin Tissoires,
Douglas Anderson, Hans de Goede, Maxime Ripard, Johan Hovold,
linux-input, linux-kernel, Radoslaw Biernacki
In-Reply-To: <CAE5UKNp3uS9cqDbQjcP3SbfxVi3wPFG4LtP6z=WU_V+M9x6LtQ@mail.gmail.com>
Hi Łukasz,
On Thu, Apr 11, 2024 at 10:23 PM Łukasz Majczak <lma@chromium.org> wrote:
>
> > Sure, but what about other transactions that are initiated by the host
> > (e.g. SET_POWER)?
> >
> Somehow it is problematic only on reboot and works just fine on
> suspend/resume and
> set_power.
> I will dig more and try to find out what the difference is.
If cold boot doesn't have such issue, maybe I2C_HID_PWR_SLEEP
shouldn't be used by reboot?
Kai-Heng
>
> > Perhaps this hack at probe is enough for your use case, but is an
> > incomplete hack and at a minimum you'd need to add a comment explaining
> > why it is there.
> >
> You mean a comment in the code ?
>
> Lukasz
^ permalink raw reply
* hid-logitech-dj support for Anywhere 3SB
From: Allan Sandfeld Jensen @ 2024-04-12 9:10 UTC (permalink / raw)
To: Filipe Laíns, linux-input
[-- Attachment #1: Type: text/plain, Size: 844 bytes --]
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
[-- Attachment #2: 0001-Logitech-Anywhere-3SB-support.patch --]
[-- Type: text/x-patch, Size: 4729 bytes --]
From 7bbe473e227ae1a16e604a3b94fa168db27fed1a 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] Logitech Anywhere 3SB support
---
Makefile | 2 +-
arch/x86/boot/Makefile | 2 +-
drivers/hid/hid-ids.h | 1 +
drivers/hid/hid-logitech-dj.c | 10 +++++++++-
drivers/hid/hid-logitech-hidpp.c | 2 ++
5 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/Makefile b/Makefile
index e00c12240191..ae2e6b53ebdf 100644
--- a/Makefile
+++ b/Makefile
@@ -2131,7 +2131,7 @@ kernelversion:
@echo $(KERNELVERSION)
image_name:
- @echo $(KBUILD_IMAGE)
+ @$(PERL) -e 'print "$(KBUILD_IMAGE)"'
quiet_cmd_rmfiles = $(if $(wildcard $(rm-files)),CLEAN $(wildcard $(rm-files)))
cmd_rmfiles = rm -rf $(rm-files)
diff --git a/arch/x86/boot/Makefile b/arch/x86/boot/Makefile
index 9e38ffaadb5d..d1043cd635dd 100644
--- a/arch/x86/boot/Makefile
+++ b/arch/x86/boot/Makefile
@@ -83,7 +83,7 @@ cmd_image = $(obj)/tools/build $(obj)/setup.bin $(obj)/vmlinux.bin \
$(obj)/bzImage: $(obj)/setup.bin $(obj)/vmlinux.bin $(obj)/tools/build FORCE
$(call if_changed,image)
- @$(kecho) 'Kernel: $@ is ready' ' (#'$(or $(KBUILD_BUILD_VERSION),`cat .version`)')'
+ $(PERL) -e 'print "Kernel: $@ is ready"'
OBJCOPYFLAGS_vmlinux.bin := -O binary -R .note -R .comment -S
$(obj)/vmlinux.bin: $(obj)/compressed/vmlinux FORCE
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
* Re: [hid:for-6.10/winwing 1/1] drivers/hid/hid-winwing.c:123:22: warning: variable 'minor' set but not used
From: Jiri Kosina @ 2024-04-12 11:17 UTC (permalink / raw)
To: kernel test robot; +Cc: Ivan Gorinov, oe-kbuild-all, linux-input
In-Reply-To: <202404041038.aQ0aRTSK-lkp@intel.com>
On Thu, 4 Apr 2024, kernel test robot wrote:
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git for-6.10/winwing
> head: 266c990debad2f9589c7a412e897a8e312b09766
> commit: 266c990debad2f9589c7a412e897a8e312b09766 [1/1] HID: Add WinWing Orion2 throttle support
> config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20240404/202404041038.aQ0aRTSK-lkp@intel.com/config)
> compiler: alpha-linux-gcc (GCC) 13.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240404/202404041038.aQ0aRTSK-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/202404041038.aQ0aRTSK-lkp@intel.com/
>
> All warnings (new ones prefixed by >>):
>
> drivers/hid/hid-winwing.c: In function 'winwing_probe':
> >> drivers/hid/hid-winwing.c:123:22: warning: variable 'minor' set but not used [-Wunused-but-set-variable]
> 123 | unsigned int minor;
> | ^~~~~
>
Ivan, will you please submit a fix for this?
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH v9 4/4] input: pm8xxx-vibrator: add new SPMI vibrator support
From: Dmitry Baryshkov @ 2024-04-12 11:35 UTC (permalink / raw)
To: Fenglin Wu
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: <633c4f8c-c22c-4128-b478-0627da8660bd@quicinc.com>
On Fri, 12 Apr 2024 at 07:05, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
>
>
>
> On 4/11/2024 10:21 PM, Dmitry Baryshkov wrote:
> > On Thu, 11 Apr 2024 at 16:51, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 2024/4/11 19:02, Dmitry Baryshkov wrote:
> >>> On Thu, 11 Apr 2024 at 11:32, Fenglin Wu via B4 Relay
> >>> <devnull+quic_fenglinw.quicinc.com@kernel.org> wrote:
> >>>>
> >>>> 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 | 51 +++++++++++++++++++++++++++++-------
> >>>> 1 file changed, 42 insertions(+), 9 deletions(-)
> >>>>
> >>>> diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
> >>>> index 2959edca8eb9..35bb6f450fd2 100644
> >>>> --- a/drivers/input/misc/pm8xxx-vibrator.c
> >>>> +++ b/drivers/input/misc/pm8xxx-vibrator.c
> >>>> @@ -12,10 +12,10 @@
> >>>> #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,6 +26,9 @@ 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;
> >>>> };
> >>>>
> >>>> @@ -45,6 +48,18 @@ static struct pm8xxx_regs pm8916_regs = {
> >>>> .drv_en_manual_mask = 0,
> >>>> };
> >>>>
> >>>> +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,
> >>>> +};
> >>>> +
> >>>> /**
> >>>> * struct pm8xxx_vib - structure to hold vibrator data
> >>>> * @vib_input_dev: input device supporting force feedback
> >>>> @@ -53,6 +68,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 +81,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 +99,10 @@ 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;
> >>>>
> >>>> + /* vibrator without drv2_addr needs be programmed in step increments */
> >>>
> >>> How are these two items related? Are you using vib->drv2_addr as a
> >>> marker for 'particular generation'? In such a case please use a flag
> >>> instead.
> >>>
> >>> The rest looks good to me.
> >>>
> >> Are you suggesting to add a flag in pm8xxx_vib as a discriminator for
> >> the new generation? I actually tried to avoid that because of this comment:
> >> https://lore.kernel.org/linux-arm-msm/ZgXSBiQcBEbwF060@google.com/#t
> >
> > Add a flag for level being programmed in steps or in mV. Using
> > drv2_addr instead of such flag is a hack.
> >
> Thanks Dmitry.
> Does this flag look good to you?
Yes, this is much better from my POV
>
> diff --git a/drivers/input/misc/pm8xxx-vibrator.c
> b/drivers/input/misc/pm8xxx-vibrator.c
> index 35bb6f450fd2..4708f441e5ac 100644
> --- a/drivers/input/misc/pm8xxx-vibrator.c
> +++ b/drivers/input/misc/pm8xxx-vibrator.c
> @@ -30,6 +30,7 @@ struct pm8xxx_regs {
> 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 = {
> @@ -37,6 +38,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 = {
> @@ -46,6 +48,7 @@ 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 = {
> @@ -58,6 +61,7 @@ static struct pm8xxx_regs pmi632_regs = {
> .drv2_mask = GENMASK(3, 0),
> .drv2_shift = 8,
> .drv_en_manual_mask = 0,
> + .drv_in_step = false,
> };
>
> /**
> @@ -100,7 +104,7 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib,
> bool on)
> const struct pm8xxx_regs *regs = vib->regs;
>
> /* vibrator without drv2_addr needs be programmed in step
> increments */
> - if (!vib->drv2_addr)
> + if (regs->drv_in_step)
> vib->level /= VIB_PER_STEP_mV(vib);
>
> if (on)
>
>
> >>
> >>>> + if (!vib->drv2_addr)
> >>>> + vib->level /= VIB_PER_STEP_mV(vib);
> >>>> +
> >>>> if (on)
> >>>> val |= (vib->level << regs->drv_shift) & regs->drv_mask;
> >>>> else
> >>>> @@ -93,6 +114,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 +147,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);
> >>>> @@ -203,6 +234,7 @@ static int pm8xxx_vib_probe(struct platform_device *pdev)
> >>>>
> >>>> 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);
> >>>> @@ -257,6 +289,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
> >>>>
> >>>>
> >>>
> >>>
> >
> >
> >
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH v9 2/4] input: pm8xxx-vibrator: refactor to support new SPMI vibrator
From: Dmitry Baryshkov @ 2024-04-12 11:36 UTC (permalink / raw)
To: Fenglin Wu
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: <00b8cea6-fed2-4224-8aeb-c731dc1b666f@quicinc.com>
On Fri, 12 Apr 2024 at 06:41, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
>
>
>
> On 4/11/2024 10:05 PM, Dmitry Baryshkov wrote:
> > On Thu, 11 Apr 2024 at 16:45, Fenglin Wu <quic_fenglinw@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 2024/4/11 18:58, Dmitry Baryshkov wrote:
> >>> On Thu, 11 Apr 2024 at 11:32, Fenglin Wu via B4 Relay
> >>> <devnull+quic_fenglinw.quicinc.com@kernel.org> wrote:
> >>>>
> >>>> 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 | 42 ++++++++++++++++++++++++------------
> >>>> 1 file changed, 28 insertions(+), 14 deletions(-)
> >>>>
> >>>> diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
> >>>> index 89f0f1c810d8..2959edca8eb9 100644
> >>>> --- a/drivers/input/misc/pm8xxx-vibrator.c
> >>>> +++ b/drivers/input/misc/pm8xxx-vibrator.c
> >>>> @@ -20,26 +20,26 @@
> >>>> #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 = 0x4A,
> >>>
> >>> If the DT already has reg = <0x4a> and you add drv_offset = 0x4a,
> >>> which register will be used by the driver?
> >>>
> >>> Also, while we are at it, please downcase all the hex numbers that you
> >>> are touching.
> >>>
> >> For SSBI vibrator, the "reg" value defined in DT is not used, see below.
> >>
> >>
> >>>> .drv_mask = 0xf8,
> >>>> .drv_shift = 3,
> >>>> .drv_en_manual_mask = 0xfc,
> >>>> };
> >>>>
> >>>> static struct pm8xxx_regs pm8916_regs = {
> >>>> - .enable_addr = 0xc046,
> >>>> + .enable_offset = 0x46,
> >>
> >> [...]
> >>
> >>>> @@ -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);
> >>>> @@ -190,13 +193,24 @@ static int pm8xxx_vib_probe(struct platform_device *pdev)
> >>>>
> >>>> regs = of_device_get_match_data(&pdev->dev);
> >>>>
> >>>> + if (regs->enable_offset != 0) {
> >>>> + 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;
> >>>> + }
> >>>> + }
> >>>> +
> >>>> + vib->enable_addr = reg_base + regs->enable_offset;
> >>>> + vib->drv_addr = reg_base + regs->drv_offset;
> >>
> >> The reg_base is initialized as 0 and it is assigned as the "reg" value
> >> defined in DT only for SPMI vibrators.
> >
> > Please don't. This is counterintuitive. We have reg in DT. We should
> > be using it.
> >
> Hmm, the original driver doesn't use the reg value defined in DT at all,
> Anyway, I can make the SSBI offset to 0, so the base address defined in
> the DT will be always added regardless of SSBI or SPMI vibrator. Let me
> know.
Yes, please. This follows the logic of the SPMI change too.
> Thanks
>
> >>
> >>>> +
> >>>> /* 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
> >>>>
> >>>>
> >>>
> >>>
> >
> >
> >
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH] HID: Remove the unused variable minor
From: Jiri Kosina @ 2024-04-12 11:38 UTC (permalink / raw)
To: Jiapeng Chong
Cc: bentiss, linux-input, linux-kernel, Abaci Robot, Ivan Gorinov
In-Reply-To: <20240407022804.27081-1-jiapeng.chong@linux.alibaba.com>
On Sun, 7 Apr 2024, Jiapeng Chong wrote:
> Variable minor is not effectively used, so delete it.
>
> drivers/hid/hid-winwing.c:123:15: warning: variable 'minor' set but not used.
>
> Reported-by: Abaci Robot <abaci@linux.alibaba.com>
> Closes: https://bugzilla.openanolis.cn/show_bug.cgi?id=8705
> Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>
I've added
Fixes: 266c990debad2 ("HID: Add WinWing Orion2 throttle support")
CCed Ivan here, and applied. Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] HID: nintendo: Remove unused function
From: Jiri Kosina @ 2024-04-12 11:40 UTC (permalink / raw)
To: Jiapeng Chong
Cc: djogorchock, bentiss, linux-input, linux-kernel, Abaci Robot
In-Reply-To: <20240407021100.48008-1-jiapeng.chong@linux.alibaba.com>
On Sun, 7 Apr 2024, Jiapeng Chong wrote:
> The function are defined in the hid-nintendo.c file, but not called
> elsewhere, so delete the unused function.
>
> drivers/hid/hid-nintendo.c:697:20: warning: unused function 'joycon_device_has_usb'.
>
> Reported-by: Abaci Robot <abaci@linux.alibaba.com>
> Closes: https://bugzilla.openanolis.cn/show_bug.cgi?id=8704
> Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>
Applied, thanks.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* 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
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