Add SW support for the vibrator module inside PMI632, PM7250B, PM7325B, PM7550BA.
It is very similar to the vibrator module inside PM8916 which is supported in
pm8xxx-vib driver but just the drive amplitude is controlled with 2 registers,
and the register base offset in each PMIC is different.
Changes in v6:
1. Add "qcom,pmi632-vib" as a standalone compatible string.
Changes in v5:
1. Drop "qcom,spmi-vib-gen2" generic compatible string as requested
and use device specific compatible strings only.
Changes in v4:
1. Update to use the combination of the HW type and register offset
as the constant match data, the register base address defined in
'reg' property will be added when accessing SPMI registers using
regmap APIs.
2. Remove 'qcom,spmi-vib-gen1' generic compatible string.
Changes in v3:
1. Refactor the driver to support different type of the vibrators with
better flexibility by introducing the HW type with corresponding
register fields definitions.
2. Add 'qcom,spmi-vib-gen1' and 'qcom,spmi-vib-gen2' compatible
strings, and add PMI632, PM7250B, PM7325B, PM7550BA as compatbile as
spmi-vib-gen2.
Changes in v2:
Remove the "pm7550ba-vib" compatible string as it's compatible with pm7325b.
Fenglin Wu (3):
input: pm8xxx-vib: refactor to easily support new SPMI vibrator
dt-bindings: input: qcom,pm8xxx-vib: add new SPMI vibrator module
input: pm8xxx-vibrator: add new SPMI vibrator support
.../bindings/input/qcom,pm8xxx-vib.yaml | 16 +-
drivers/input/misc/pm8xxx-vibrator.c | 171 ++++++++++++------
2 files changed, 132 insertions(+), 55 deletions(-)
--
2.25.1
Add new SPMI vibrator module which is very similar to the SPMI vibrator
module inside PM8916 but just has a finer drive voltage step (1mV vs
100mV) hence its drive level control is expanded to across 2 registers.
The vibrator module can be found in Qualcomm PMIC PMI632, then following
PM7250B, PM7325B, PM7550BA PMICs.
Signed-off-by: Fenglin Wu <[email protected]>
Tested-by: Luca Weiss <[email protected]> # sdm632-fairphone-fp3 (pmi632)
---
drivers/input/misc/pm8xxx-vibrator.c | 55 +++++++++++++++++++++++++---
1 file changed, 50 insertions(+), 5 deletions(-)
diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
index d6b468324c77..990e8a9ac018 100644
--- a/drivers/input/misc/pm8xxx-vibrator.c
+++ b/drivers/input/misc/pm8xxx-vibrator.c
@@ -21,6 +21,13 @@
#define SPMI_VIB_DRV_LEVEL_MASK GENMASK(4, 0)
#define SPMI_VIB_DRV_SHIFT 0
+#define SPMI_VIB_GEN2_DRV_REG 0x40
+#define SPMI_VIB_GEN2_DRV_MASK GENMASK(7, 0)
+#define SPMI_VIB_GEN2_DRV_SHIFT 0
+#define SPMI_VIB_GEN2_DRV2_REG 0x41
+#define SPMI_VIB_GEN2_DRV2_MASK GENMASK(3, 0)
+#define SPMI_VIB_GEN2_DRV2_SHIFT 8
+
#define SPMI_VIB_EN_REG 0x46
#define SPMI_VIB_EN_BIT BIT(7)
@@ -33,12 +40,14 @@
enum vib_hw_type {
SSBI_VIB,
SPMI_VIB,
+ SPMI_VIB_GEN2
};
struct pm8xxx_vib_data {
enum vib_hw_type hw_type;
unsigned int enable_addr;
unsigned int drv_addr;
+ unsigned int drv2_addr;
};
static const struct pm8xxx_vib_data ssbi_vib_data = {
@@ -52,6 +61,13 @@ static const struct pm8xxx_vib_data spmi_vib_data = {
.drv_addr = SPMI_VIB_DRV_REG,
};
+static const struct pm8xxx_vib_data spmi_vib_gen2_data = {
+ .hw_type = SPMI_VIB_GEN2,
+ .enable_addr = SPMI_VIB_EN_REG,
+ .drv_addr = SPMI_VIB_GEN2_DRV_REG,
+ .drv2_addr = SPMI_VIB_GEN2_DRV2_REG,
+};
+
/**
* struct pm8xxx_vib - structure to hold vibrator data
* @vib_input_dev: input device supporting force feedback
@@ -85,12 +101,24 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
{
int rc;
unsigned int val = vib->reg_vib_drv;
- u32 mask = SPMI_VIB_DRV_LEVEL_MASK;
- u32 shift = SPMI_VIB_DRV_SHIFT;
+ u32 mask, shift;
- if (vib->data->hw_type == SSBI_VIB) {
+
+ switch (vib->data->hw_type) {
+ case SSBI_VIB:
mask = SSBI_VIB_DRV_LEVEL_MASK;
shift = SSBI_VIB_DRV_SHIFT;
+ break;
+ case SPMI_VIB:
+ mask = SPMI_VIB_DRV_LEVEL_MASK;
+ shift = SPMI_VIB_DRV_SHIFT;
+ break;
+ case SPMI_VIB_GEN2:
+ mask = SPMI_VIB_GEN2_DRV_MASK;
+ shift = SPMI_VIB_GEN2_DRV_SHIFT;
+ break;
+ default:
+ return -EINVAL;
}
if (on)
@@ -104,6 +132,19 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
vib->reg_vib_drv = val;
+ if (vib->data->hw_type == SPMI_VIB_GEN2) {
+ mask = SPMI_VIB_GEN2_DRV2_MASK;
+ shift = SPMI_VIB_GEN2_DRV2_SHIFT;
+ if (on)
+ val = (vib->level >> shift) & mask;
+ else
+ val = 0;
+ rc = regmap_update_bits(vib->regmap,
+ vib->reg_base + vib->data->drv2_addr, mask, val);
+ if (rc < 0)
+ return rc;
+ }
+
if (vib->data->hw_type == SSBI_VIB)
return 0;
@@ -128,10 +169,13 @@ static void pm8xxx_work_handler(struct work_struct *work)
vib->active = true;
vib->level = ((VIB_MAX_LEVELS * vib->speed) / MAX_FF_SPEED) +
VIB_MIN_LEVEL_mV;
- vib->level /= 100;
+ if (vib->data->hw_type != SPMI_VIB_GEN2)
+ vib->level /= 100;
} else {
vib->active = false;
- vib->level = VIB_MIN_LEVEL_mV / 100;
+ vib->level = VIB_MIN_LEVEL_mV;
+ if (vib->data->hw_type != SPMI_VIB_GEN2)
+ vib->level /= 100;
}
pm8xxx_vib_set(vib, vib->active);
@@ -266,6 +310,7 @@ static const struct of_device_id pm8xxx_vib_id_table[] = {
{ .compatible = "qcom,pm8058-vib", .data = &ssbi_vib_data },
{ .compatible = "qcom,pm8921-vib", .data = &ssbi_vib_data },
{ .compatible = "qcom,pm8916-vib", .data = &spmi_vib_data },
+ { .compatible = "qcom,pmi632-vib", .data = &spmi_vib_gen2_data },
{ }
};
MODULE_DEVICE_TABLE(of, pm8xxx_vib_id_table);
--
2.25.1
Currently, all vibrator control register addresses are hard coded,
including the base address and the offset, it's not flexible to support
new SPMI vibrator module which is usually included in different PMICs
with different base address. Refactor this by defining register offset
with HW type combination, and register base address which is defined
in 'reg' property is added for SPMI vibrators.
Signed-off-by: Fenglin Wu <[email protected]>
---
drivers/input/misc/pm8xxx-vibrator.c | 122 ++++++++++++++++-----------
1 file changed, 73 insertions(+), 49 deletions(-)
diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
index 04cb87efd799..d6b468324c77 100644
--- a/drivers/input/misc/pm8xxx-vibrator.c
+++ b/drivers/input/misc/pm8xxx-vibrator.c
@@ -12,36 +12,44 @@
#include <linux/regmap.h>
#include <linux/slab.h>
+#define SSBL_VIB_DRV_REG 0x4A
+#define SSBI_VIB_DRV_EN_MANUAL_MASK GENMASK(7, 2)
+#define SSBI_VIB_DRV_LEVEL_MASK GENMASK(7, 3)
+#define SSBI_VIB_DRV_SHIFT 3
+
+#define SPMI_VIB_DRV_REG 0x41
+#define SPMI_VIB_DRV_LEVEL_MASK GENMASK(4, 0)
+#define SPMI_VIB_DRV_SHIFT 0
+
+#define SPMI_VIB_EN_REG 0x46
+#define SPMI_VIB_EN_BIT BIT(7)
+
#define VIB_MAX_LEVEL_mV (3100)
#define VIB_MIN_LEVEL_mV (1200)
#define VIB_MAX_LEVELS (VIB_MAX_LEVEL_mV - VIB_MIN_LEVEL_mV)
#define MAX_FF_SPEED 0xff
-struct pm8xxx_regs {
- unsigned int enable_addr;
- unsigned int enable_mask;
+enum vib_hw_type {
+ SSBI_VIB,
+ SPMI_VIB,
+};
- unsigned int drv_addr;
- unsigned int drv_mask;
- unsigned int drv_shift;
- unsigned int drv_en_manual_mask;
+struct pm8xxx_vib_data {
+ enum vib_hw_type hw_type;
+ unsigned int enable_addr;
+ unsigned int drv_addr;
};
-static const struct pm8xxx_regs pm8058_regs = {
- .drv_addr = 0x4A,
- .drv_mask = 0xf8,
- .drv_shift = 3,
- .drv_en_manual_mask = 0xfc,
+static const struct pm8xxx_vib_data ssbi_vib_data = {
+ .hw_type = SSBI_VIB,
+ .drv_addr = SSBL_VIB_DRV_REG,
};
-static struct pm8xxx_regs pm8916_regs = {
- .enable_addr = 0xc046,
- .enable_mask = BIT(7),
- .drv_addr = 0xc041,
- .drv_mask = 0x1F,
- .drv_shift = 0,
- .drv_en_manual_mask = 0,
+static const struct pm8xxx_vib_data spmi_vib_data = {
+ .hw_type = SPMI_VIB,
+ .enable_addr = SPMI_VIB_EN_REG,
+ .drv_addr = SPMI_VIB_DRV_REG,
};
/**
@@ -49,7 +57,8 @@ static struct pm8xxx_regs pm8916_regs = {
* @vib_input_dev: input device supporting force feedback
* @work: work structure to set the vibration parameters
* @regmap: regmap for register read/write
- * @regs: registers' info
+ * @data: vibrator HW info
+ * @reg_base: the register base of the module
* @speed: speed of vibration set from userland
* @active: state of vibrator
* @level: level of vibration to set in the chip
@@ -59,7 +68,8 @@ struct pm8xxx_vib {
struct input_dev *vib_input_dev;
struct work_struct work;
struct regmap *regmap;
- const struct pm8xxx_regs *regs;
+ const struct pm8xxx_vib_data *data;
+ unsigned int reg_base;
int speed;
int level;
bool active;
@@ -75,24 +85,31 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
{
int rc;
unsigned int val = vib->reg_vib_drv;
- const struct pm8xxx_regs *regs = vib->regs;
+ u32 mask = SPMI_VIB_DRV_LEVEL_MASK;
+ u32 shift = SPMI_VIB_DRV_SHIFT;
+
+ if (vib->data->hw_type == SSBI_VIB) {
+ mask = SSBI_VIB_DRV_LEVEL_MASK;
+ shift = SSBI_VIB_DRV_SHIFT;
+ }
if (on)
- val |= (vib->level << regs->drv_shift) & regs->drv_mask;
+ val |= (vib->level << shift) & mask;
else
- val &= ~regs->drv_mask;
+ val &= ~mask;
- rc = regmap_write(vib->regmap, regs->drv_addr, val);
+ rc = regmap_update_bits(vib->regmap, vib->reg_base + vib->data->drv_addr, mask, val);
if (rc < 0)
return rc;
vib->reg_vib_drv = val;
- if (regs->enable_mask)
- rc = regmap_update_bits(vib->regmap, regs->enable_addr,
- regs->enable_mask, on ? ~0 : 0);
+ if (vib->data->hw_type == SSBI_VIB)
+ return 0;
- return rc;
+ mask = SPMI_VIB_EN_BIT;
+ val = on ? SPMI_VIB_EN_BIT : 0;
+ return regmap_update_bits(vib->regmap, vib->reg_base + vib->data->enable_addr, mask, val);
}
/**
@@ -102,13 +119,6 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
static void pm8xxx_work_handler(struct work_struct *work)
{
struct pm8xxx_vib *vib = container_of(work, struct pm8xxx_vib, work);
- const struct pm8xxx_regs *regs = vib->regs;
- int rc;
- unsigned int val;
-
- rc = regmap_read(vib->regmap, regs->drv_addr, &val);
- if (rc < 0)
- return;
/*
* pmic vibrator supports voltage ranges from 1.2 to 3.1V, so
@@ -168,9 +178,9 @@ static int pm8xxx_vib_probe(struct platform_device *pdev)
{
struct pm8xxx_vib *vib;
struct input_dev *input_dev;
+ const struct pm8xxx_vib_data *data;
int error;
- unsigned int val;
- const struct pm8xxx_regs *regs;
+ unsigned int val, reg_base;
vib = devm_kzalloc(&pdev->dev, sizeof(*vib), GFP_KERNEL);
if (!vib)
@@ -187,19 +197,33 @@ static int pm8xxx_vib_probe(struct platform_device *pdev)
INIT_WORK(&vib->work, pm8xxx_work_handler);
vib->vib_input_dev = input_dev;
- regs = of_device_get_match_data(&pdev->dev);
+ data = of_device_get_match_data(&pdev->dev);
+ if (!data)
+ return -EINVAL;
- /* operate in manual mode */
- error = regmap_read(vib->regmap, regs->drv_addr, &val);
- if (error < 0)
- return error;
+ if (data->hw_type != SSBI_VIB) {
+ 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->reg_base += reg_base;
+ }
- val &= regs->drv_en_manual_mask;
- error = regmap_write(vib->regmap, regs->drv_addr, val);
+ error = regmap_read(vib->regmap, vib->reg_base + data->drv_addr, &val);
if (error < 0)
return error;
- vib->regs = regs;
+ /* operate in manual mode */
+ if (data->hw_type == SSBI_VIB) {
+ val &= SSBI_VIB_DRV_EN_MANUAL_MASK;
+ error = regmap_write(vib->regmap, vib->reg_base + data->drv_addr, val);
+ if (error < 0)
+ return error;
+ }
+
+ vib->data = data;
vib->reg_vib_drv = val;
input_dev->name = "pm8xxx_vib_ffmemless";
@@ -239,9 +263,9 @@ static int pm8xxx_vib_suspend(struct device *dev)
static DEFINE_SIMPLE_DEV_PM_OPS(pm8xxx_vib_pm_ops, pm8xxx_vib_suspend, NULL);
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,pm8058-vib", .data = &ssbi_vib_data },
+ { .compatible = "qcom,pm8921-vib", .data = &ssbi_vib_data },
+ { .compatible = "qcom,pm8916-vib", .data = &spmi_vib_data },
{ }
};
MODULE_DEVICE_TABLE(of, pm8xxx_vib_id_table);
--
2.25.1
On Fri, 22 Sept 2023 at 11:38, Fenglin Wu <[email protected]> wrote:
>
> Currently, all vibrator control register addresses are hard coded,
> including the base address and the offset, it's not flexible to support
> new SPMI vibrator module which is usually included in different PMICs
> with different base address. Refactor this by defining register offset
> with HW type combination, and register base address which is defined
> in 'reg' property is added for SPMI vibrators.
>
> Signed-off-by: Fenglin Wu <[email protected]>
> ---
> drivers/input/misc/pm8xxx-vibrator.c | 122 ++++++++++++++++-----------
> 1 file changed, 73 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
> index 04cb87efd799..d6b468324c77 100644
> --- a/drivers/input/misc/pm8xxx-vibrator.c
> +++ b/drivers/input/misc/pm8xxx-vibrator.c
> @@ -12,36 +12,44 @@
> #include <linux/regmap.h>
> #include <linux/slab.h>
>
> +#define SSBL_VIB_DRV_REG 0x4A
SSBI_VIB....
> +#define SSBI_VIB_DRV_EN_MANUAL_MASK GENMASK(7, 2)
> +#define SSBI_VIB_DRV_LEVEL_MASK GENMASK(7, 3)
> +#define SSBI_VIB_DRV_SHIFT 3
> +
> +#define SPMI_VIB_DRV_REG 0x41
> +#define SPMI_VIB_DRV_LEVEL_MASK GENMASK(4, 0)
> +#define SPMI_VIB_DRV_SHIFT 0
> +
> +#define SPMI_VIB_EN_REG 0x46
> +#define SPMI_VIB_EN_BIT BIT(7)
> +
> #define VIB_MAX_LEVEL_mV (3100)
> #define VIB_MIN_LEVEL_mV (1200)
> #define VIB_MAX_LEVELS (VIB_MAX_LEVEL_mV - VIB_MIN_LEVEL_mV)
>
> #define MAX_FF_SPEED 0xff
>
> -struct pm8xxx_regs {
> - unsigned int enable_addr;
> - unsigned int enable_mask;
> +enum vib_hw_type {
> + SSBI_VIB,
> + SPMI_VIB,
> +};
>
> - unsigned int drv_addr;
> - unsigned int drv_mask;
> - unsigned int drv_shift;
> - unsigned int drv_en_manual_mask;
> +struct pm8xxx_vib_data {
> + enum vib_hw_type hw_type;
> + unsigned int enable_addr;
> + unsigned int drv_addr;
> };
>
> -static const struct pm8xxx_regs pm8058_regs = {
> - .drv_addr = 0x4A,
> - .drv_mask = 0xf8,
> - .drv_shift = 3,
> - .drv_en_manual_mask = 0xfc,
> +static const struct pm8xxx_vib_data ssbi_vib_data = {
> + .hw_type = SSBI_VIB,
> + .drv_addr = SSBL_VIB_DRV_REG,
> };
>
> -static struct pm8xxx_regs pm8916_regs = {
> - .enable_addr = 0xc046,
> - .enable_mask = BIT(7),
> - .drv_addr = 0xc041,
> - .drv_mask = 0x1F,
> - .drv_shift = 0,
> - .drv_en_manual_mask = 0,
> +static const struct pm8xxx_vib_data spmi_vib_data = {
> + .hw_type = SPMI_VIB,
> + .enable_addr = SPMI_VIB_EN_REG,
> + .drv_addr = SPMI_VIB_DRV_REG,
> };
>
> /**
> @@ -49,7 +57,8 @@ static struct pm8xxx_regs pm8916_regs = {
> * @vib_input_dev: input device supporting force feedback
> * @work: work structure to set the vibration parameters
> * @regmap: regmap for register read/write
> - * @regs: registers' info
> + * @data: vibrator HW info
> + * @reg_base: the register base of the module
> * @speed: speed of vibration set from userland
> * @active: state of vibrator
> * @level: level of vibration to set in the chip
> @@ -59,7 +68,8 @@ struct pm8xxx_vib {
> struct input_dev *vib_input_dev;
> struct work_struct work;
> struct regmap *regmap;
> - const struct pm8xxx_regs *regs;
> + const struct pm8xxx_vib_data *data;
> + unsigned int reg_base;
> int speed;
> int level;
> bool active;
> @@ -75,24 +85,31 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
> {
> int rc;
> unsigned int val = vib->reg_vib_drv;
> - const struct pm8xxx_regs *regs = vib->regs;
> + u32 mask = SPMI_VIB_DRV_LEVEL_MASK;
> + u32 shift = SPMI_VIB_DRV_SHIFT;
> +
> + if (vib->data->hw_type == SSBI_VIB) {
> + mask = SSBI_VIB_DRV_LEVEL_MASK;
> + shift = SSBI_VIB_DRV_SHIFT;
> + }
>
> if (on)
> - val |= (vib->level << regs->drv_shift) & regs->drv_mask;
> + val |= (vib->level << shift) & mask;
> else
> - val &= ~regs->drv_mask;
> + val &= ~mask;
>
> - rc = regmap_write(vib->regmap, regs->drv_addr, val);
> + rc = regmap_update_bits(vib->regmap, vib->reg_base + vib->data->drv_addr, mask, val);
> if (rc < 0)
> return rc;
>
> vib->reg_vib_drv = val;
>
> - if (regs->enable_mask)
> - rc = regmap_update_bits(vib->regmap, regs->enable_addr,
> - regs->enable_mask, on ? ~0 : 0);
> + if (vib->data->hw_type == SSBI_VIB)
> + return 0;
>
> - return rc;
> + mask = SPMI_VIB_EN_BIT;
> + val = on ? SPMI_VIB_EN_BIT : 0;
> + return regmap_update_bits(vib->regmap, vib->reg_base + vib->data->enable_addr, mask, val);
> }
>
> /**
> @@ -102,13 +119,6 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
> static void pm8xxx_work_handler(struct work_struct *work)
> {
> struct pm8xxx_vib *vib = container_of(work, struct pm8xxx_vib, work);
> - const struct pm8xxx_regs *regs = vib->regs;
> - int rc;
> - unsigned int val;
> -
> - rc = regmap_read(vib->regmap, regs->drv_addr, &val);
> - if (rc < 0)
> - return;
>
> /*
> * pmic vibrator supports voltage ranges from 1.2 to 3.1V, so
> @@ -168,9 +178,9 @@ static int pm8xxx_vib_probe(struct platform_device *pdev)
> {
> struct pm8xxx_vib *vib;
> struct input_dev *input_dev;
> + const struct pm8xxx_vib_data *data;
> int error;
> - unsigned int val;
> - const struct pm8xxx_regs *regs;
> + unsigned int val, reg_base;
>
> vib = devm_kzalloc(&pdev->dev, sizeof(*vib), GFP_KERNEL);
> if (!vib)
> @@ -187,19 +197,33 @@ static int pm8xxx_vib_probe(struct platform_device *pdev)
> INIT_WORK(&vib->work, pm8xxx_work_handler);
> vib->vib_input_dev = input_dev;
>
> - regs = of_device_get_match_data(&pdev->dev);
> + data = of_device_get_match_data(&pdev->dev);
> + if (!data)
> + return -EINVAL;
>
> - /* operate in manual mode */
> - error = regmap_read(vib->regmap, regs->drv_addr, &val);
> - if (error < 0)
> - return error;
> + if (data->hw_type != SSBI_VIB) {
You can drop this condition, if ssbi_vib_data.drv_addr is 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->reg_base += reg_base;
> + }
>
> - val &= regs->drv_en_manual_mask;
> - error = regmap_write(vib->regmap, regs->drv_addr, val);
> + error = regmap_read(vib->regmap, vib->reg_base + data->drv_addr, &val);
> if (error < 0)
> return error;
>
> - vib->regs = regs;
> + /* operate in manual mode */
> + if (data->hw_type == SSBI_VIB) {
> + val &= SSBI_VIB_DRV_EN_MANUAL_MASK;
> + error = regmap_write(vib->regmap, vib->reg_base + data->drv_addr, val);
> + if (error < 0)
> + return error;
> + }
> +
> + vib->data = data;
> vib->reg_vib_drv = val;
>
> input_dev->name = "pm8xxx_vib_ffmemless";
> @@ -239,9 +263,9 @@ static int pm8xxx_vib_suspend(struct device *dev)
> static DEFINE_SIMPLE_DEV_PM_OPS(pm8xxx_vib_pm_ops, pm8xxx_vib_suspend, NULL);
>
> 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,pm8058-vib", .data = &ssbi_vib_data },
> + { .compatible = "qcom,pm8921-vib", .data = &ssbi_vib_data },
> + { .compatible = "qcom,pm8916-vib", .data = &spmi_vib_data },
> { }
> };
> MODULE_DEVICE_TABLE(of, pm8xxx_vib_id_table);
> --
> 2.25.1
>
--
With best wishes
Dmitry
On Fri, 22 Sept 2023 at 11:38, Fenglin Wu <[email protected]> wrote:
>
> Add new SPMI vibrator module which is very similar to the SPMI vibrator
> module inside PM8916 but just has a finer drive voltage step (1mV vs
> 100mV) hence its drive level control is expanded to across 2 registers.
> The vibrator module can be found in Qualcomm PMIC PMI632, then following
> PM7250B, PM7325B, PM7550BA PMICs.
>
> Signed-off-by: Fenglin Wu <[email protected]>
> Tested-by: Luca Weiss <[email protected]> # sdm632-fairphone-fp3 (pmi632)
> ---
> drivers/input/misc/pm8xxx-vibrator.c | 55 +++++++++++++++++++++++++---
> 1 file changed, 50 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
> index d6b468324c77..990e8a9ac018 100644
> --- a/drivers/input/misc/pm8xxx-vibrator.c
> +++ b/drivers/input/misc/pm8xxx-vibrator.c
> @@ -21,6 +21,13 @@
> #define SPMI_VIB_DRV_LEVEL_MASK GENMASK(4, 0)
> #define SPMI_VIB_DRV_SHIFT 0
>
> +#define SPMI_VIB_GEN2_DRV_REG 0x40
> +#define SPMI_VIB_GEN2_DRV_MASK GENMASK(7, 0)
> +#define SPMI_VIB_GEN2_DRV_SHIFT 0
> +#define SPMI_VIB_GEN2_DRV2_REG 0x41
> +#define SPMI_VIB_GEN2_DRV2_MASK GENMASK(3, 0)
> +#define SPMI_VIB_GEN2_DRV2_SHIFT 8
> +
> #define SPMI_VIB_EN_REG 0x46
> #define SPMI_VIB_EN_BIT BIT(7)
>
> @@ -33,12 +40,14 @@
> enum vib_hw_type {
> SSBI_VIB,
> SPMI_VIB,
> + SPMI_VIB_GEN2
> };
>
> struct pm8xxx_vib_data {
> enum vib_hw_type hw_type;
> unsigned int enable_addr;
> unsigned int drv_addr;
> + unsigned int drv2_addr;
> };
>
> static const struct pm8xxx_vib_data ssbi_vib_data = {
> @@ -52,6 +61,13 @@ static const struct pm8xxx_vib_data spmi_vib_data = {
> .drv_addr = SPMI_VIB_DRV_REG,
> };
>
> +static const struct pm8xxx_vib_data spmi_vib_gen2_data = {
> + .hw_type = SPMI_VIB_GEN2,
> + .enable_addr = SPMI_VIB_EN_REG,
> + .drv_addr = SPMI_VIB_GEN2_DRV_REG,
> + .drv2_addr = SPMI_VIB_GEN2_DRV2_REG,
> +};
> +
> /**
> * struct pm8xxx_vib - structure to hold vibrator data
> * @vib_input_dev: input device supporting force feedback
> @@ -85,12 +101,24 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
> {
> int rc;
> unsigned int val = vib->reg_vib_drv;
> - u32 mask = SPMI_VIB_DRV_LEVEL_MASK;
> - u32 shift = SPMI_VIB_DRV_SHIFT;
> + u32 mask, shift;
>
> - if (vib->data->hw_type == SSBI_VIB) {
> +
> + switch (vib->data->hw_type) {
> + case SSBI_VIB:
> mask = SSBI_VIB_DRV_LEVEL_MASK;
> shift = SSBI_VIB_DRV_SHIFT;
> + break;
> + case SPMI_VIB:
> + mask = SPMI_VIB_DRV_LEVEL_MASK;
> + shift = SPMI_VIB_DRV_SHIFT;
> + break;
> + case SPMI_VIB_GEN2:
> + mask = SPMI_VIB_GEN2_DRV_MASK;
> + shift = SPMI_VIB_GEN2_DRV_SHIFT;
> + break;
> + default:
> + return -EINVAL;
Could you please move the switch to the previous patch? Then it would
be more obvious that you are just adding the SPMI_VIB_GEN2 here.
Other than that LGTM.
> }
>
> if (on)
> @@ -104,6 +132,19 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
>
> vib->reg_vib_drv = val;
>
> + if (vib->data->hw_type == SPMI_VIB_GEN2) {
> + mask = SPMI_VIB_GEN2_DRV2_MASK;
> + shift = SPMI_VIB_GEN2_DRV2_SHIFT;
> + if (on)
> + val = (vib->level >> shift) & mask;
> + else
> + val = 0;
> + rc = regmap_update_bits(vib->regmap,
> + vib->reg_base + vib->data->drv2_addr, mask, val);
> + if (rc < 0)
> + return rc;
> + }
> +
> if (vib->data->hw_type == SSBI_VIB)
> return 0;
>
> @@ -128,10 +169,13 @@ static void pm8xxx_work_handler(struct work_struct *work)
> vib->active = true;
> vib->level = ((VIB_MAX_LEVELS * vib->speed) / MAX_FF_SPEED) +
> VIB_MIN_LEVEL_mV;
> - vib->level /= 100;
> + if (vib->data->hw_type != SPMI_VIB_GEN2)
> + vib->level /= 100;
> } else {
> vib->active = false;
> - vib->level = VIB_MIN_LEVEL_mV / 100;
> + vib->level = VIB_MIN_LEVEL_mV;
> + if (vib->data->hw_type != SPMI_VIB_GEN2)
> + vib->level /= 100;
> }
>
> pm8xxx_vib_set(vib, vib->active);
> @@ -266,6 +310,7 @@ static const struct of_device_id pm8xxx_vib_id_table[] = {
> { .compatible = "qcom,pm8058-vib", .data = &ssbi_vib_data },
> { .compatible = "qcom,pm8921-vib", .data = &ssbi_vib_data },
> { .compatible = "qcom,pm8916-vib", .data = &spmi_vib_data },
> + { .compatible = "qcom,pmi632-vib", .data = &spmi_vib_gen2_data },
> { }
> };
> MODULE_DEVICE_TABLE(of, pm8xxx_vib_id_table);
> --
> 2.25.1
>
--
With best wishes
Dmitry
On 9/24/2023 3:07 AM, Dmitry Baryshkov wrote:
>> +
>> + switch (vib->data->hw_type) {
>> + case SSBI_VIB:
>> mask = SSBI_VIB_DRV_LEVEL_MASK;
>> shift = SSBI_VIB_DRV_SHIFT;
>> + break;
>> + case SPMI_VIB:
>> + mask = SPMI_VIB_DRV_LEVEL_MASK;
>> + shift = SPMI_VIB_DRV_SHIFT;
>> + break;
>> + case SPMI_VIB_GEN2:
>> + mask = SPMI_VIB_GEN2_DRV_MASK;
>> + shift = SPMI_VIB_GEN2_DRV_SHIFT;
>> + break;
>> + default:
>> + return -EINVAL;
> Could you please move the switch to the previous patch? Then it would
> be more obvious that you are just adding the SPMI_VIB_GEN2 here.
>
> Other than that LGTM.
Sure, I can move the switch to the previous refactoring patch.
>
>> }
On 9/24/2023 3:05 AM, Dmitry Baryshkov wrote:
>> +#define SSBL_VIB_DRV_REG 0x4A
> SSBI_VIB....
>
Thanks for catching the typo, I will fix it in next patch.
>> +#define SSBI_VIB_DRV_EN_MANUAL_MASK GENMASK(7, 2)
>> - /* operate in manual mode */
>> - error = regmap_read(vib->regmap, regs->drv_addr, &val);
>> - if (error < 0)
>> - return error;
>> + if (data->hw_type != SSBI_VIB) {
> You can drop this condition, if ssbi_vib_data.drv_addr is 0.
I am not sure if I understood this comment: 1st, ssbi_vib_data.drv_addr
is defined as a constant value 0x4A, so it would never be 0. 2nd, The
condition check here is to ignore reading the register base address for
SSBI_VIB HW, so we should do the check based on the HW type.
>
>> + 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->reg_base += reg_base;
On Mon, Sep 25, 2023 at 10:54:45AM +0800, Fenglin Wu wrote:
>
>
> On 9/24/2023 3:07 AM, Dmitry Baryshkov wrote:
> > > +
> > > + switch (vib->data->hw_type) {
> > > + case SSBI_VIB:
> > > mask = SSBI_VIB_DRV_LEVEL_MASK;
> > > shift = SSBI_VIB_DRV_SHIFT;
> > > + break;
> > > + case SPMI_VIB:
> > > + mask = SPMI_VIB_DRV_LEVEL_MASK;
> > > + shift = SPMI_VIB_DRV_SHIFT;
> > > + break;
> > > + case SPMI_VIB_GEN2:
> > > + mask = SPMI_VIB_GEN2_DRV_MASK;
> > > + shift = SPMI_VIB_GEN2_DRV_SHIFT;
> > > + break;
> > > + default:
> > > + return -EINVAL;
> > Could you please move the switch to the previous patch? Then it would
> > be more obvious that you are just adding the SPMI_VIB_GEN2 here.
> >
> > Other than that LGTM.
>
> Sure, I can move the switch to the previous refactoring patch.
Actually, the idea of having a const "reg" or "chip", etc. structure is
to avoid this kind of runtime checks based on hardware type and instead
use common computation. I believe you need to move mask and shift into
the chip-specific structure and avoid defining hw_type.
Thanks.
--
Dmitry
On 10/1/2023 12:17 AM, Dmitry Torokhov wrote:
> On Mon, Sep 25, 2023 at 10:54:45AM +0800, Fenglin Wu wrote:
>>
>>
>> On 9/24/2023 3:07 AM, Dmitry Baryshkov wrote:
>>>> +
>>>> + switch (vib->data->hw_type) {
>>>> + case SSBI_VIB:
>>>> mask = SSBI_VIB_DRV_LEVEL_MASK;
>>>> shift = SSBI_VIB_DRV_SHIFT;
>>>> + break;
>>>> + case SPMI_VIB:
>>>> + mask = SPMI_VIB_DRV_LEVEL_MASK;
>>>> + shift = SPMI_VIB_DRV_SHIFT;
>>>> + break;
>>>> + case SPMI_VIB_GEN2:
>>>> + mask = SPMI_VIB_GEN2_DRV_MASK;
>>>> + shift = SPMI_VIB_GEN2_DRV_SHIFT;
>>>> + break;
>>>> + default:
>>>> + return -EINVAL;
>>> Could you please move the switch to the previous patch? Then it would
>>> be more obvious that you are just adding the SPMI_VIB_GEN2 here.
>>>
>>> Other than that LGTM.
>>
>> Sure, I can move the switch to the previous refactoring patch.
>
> Actually, the idea of having a const "reg" or "chip", etc. structure is
> to avoid this kind of runtime checks based on hardware type and instead
> use common computation. I believe you need to move mask and shift into
> the chip-specific structure and avoid defining hw_type.
>
Actually, the main motivation for adding 'hw_type' is to avoid reading
'reg_base' from DT for SSBI_VIB. It can also help to simplify the
'pm8xxx_vib_data' structure and make following code logic more
straightforward and easier to understand(check hw_type instead of
checking specific constant reg/mask value), it has been used in
following places:
1) Avoid reading 'reg_base' from DT for SSBI_VIB.
2) Only do manual-mode-mask-write for SSBI_VIB. Previously, it was
achieved by giving a valid 'drv_en_manual_mask' value only for SSBI_VIB,
having hw_type make it more straightforward.
3) Not writing VIB_EN register for SSBI_VIB. A similar strategy was
used previously, only write VIB_EN register when 'enable_mask' is valid,
checking hw_type make it more straightforward.
4) To achieve different drive step size for SPMI_VIB （100mV per step)
and SPMI_VIB_GEN2 (1mV per step).
5) Do different VIB_DRV mask and shift assignment for SPMI_VIB and
SPMI_VIB_GEN2
6) Only write VIB_DRV2 for SPMI_VIB_GEN2.
> Thanks.
>
On 10/9/2023 12:01 PM, Fenglin Wu wrote:
>
>
> On 10/1/2023 12:17 AM, Dmitry Torokhov wrote:
>> On Mon, Sep 25, 2023 at 10:54:45AM +0800, Fenglin Wu wrote:
>>>
>>>
>>> On 9/24/2023 3:07 AM, Dmitry Baryshkov wrote:
>>>>> +
>>>>> + switch (vib->data->hw_type) {
>>>>> + case SSBI_VIB:
>>>>> mask = SSBI_VIB_DRV_LEVEL_MASK;
>>>>> shift = SSBI_VIB_DRV_SHIFT;
>>>>> + break;
>>>>> + case SPMI_VIB:
>>>>> + mask = SPMI_VIB_DRV_LEVEL_MASK;
>>>>> + shift = SPMI_VIB_DRV_SHIFT;
>>>>> + break;
>>>>> + case SPMI_VIB_GEN2:
>>>>> + mask = SPMI_VIB_GEN2_DRV_MASK;
>>>>> + shift = SPMI_VIB_GEN2_DRV_SHIFT;
>>>>> + break;
>>>>> + default:
>>>>> + return -EINVAL;
>>>> Could you please move the switch to the previous patch? Then it would
>>>> be more obvious that you are just adding the SPMI_VIB_GEN2 here.
>>>>
>>>> Other than that LGTM.
>>>
>>> Sure, I can move the switch to the previous refactoring patch.
>>
>> Actually, the idea of having a const "reg" or "chip", etc. structure is
>> to avoid this kind of runtime checks based on hardware type and instead
>> use common computation. I believe you need to move mask and shift into
>> the chip-specific structure and avoid defining hw_type.
>>
>
> Actually, the main motivation for adding 'hw_type' is to avoid reading
> 'reg_base' from DT for SSBI_VIB. It can also help to simplify the
> 'pm8xxx_vib_data' structure and make following code logic more
> straightforward and easier to understand(check hw_type instead of
> checking specific constant reg/mask value), it has been used in
> following places:
>
> 1) Avoid reading 'reg_base' from DT for SSBI_VIB.
> 2) Only do manual-mode-mask-write for SSBI_VIB. Previously, it was
> achieved by giving a valid 'drv_en_manual_mask' value only for SSBI_VIB,
> having hw_type make it more straightforward.
> 3) Not writing VIB_EN register for SSBI_VIB. A similar strategy was
> used previously, only write VIB_EN register when 'enable_mask' is valid,
> checking hw_type make it more straightforward.
> 4) To achieve different drive step size for SPMI_VIB （100mV per
> step) and SPMI_VIB_GEN2 (1mV per step).
> 5) Do different VIB_DRV mask and shift assignment for SPMI_VIB and
> SPMI_VIB_GEN2
> 6) Only write VIB_DRV2 for SPMI_VIB_GEN2.
>
Hi Dmitry,
Can you please help to comment if this looks good for you?
I actually have pushed a V7 to address your last comment before you made
this one.
V7 change:
https://lore.kernel.org/linux-arm-msm/[email protected]/,
just want to know how to move forward.
Thanks
Fenglin
>
>> Thanks.
>>
On 2023/10/1 0:17, Dmitry Torokhov wrote:
> On Mon, Sep 25, 2023 at 10:54:45AM +0800, Fenglin Wu wrote:
>>
>>
>> On 9/24/2023 3:07 AM, Dmitry Baryshkov wrote:
>>>> +
>>>> + switch (vib->data->hw_type) {
>>>> + case SSBI_VIB:
>>>> mask = SSBI_VIB_DRV_LEVEL_MASK;
>>>> shift = SSBI_VIB_DRV_SHIFT;
>>>> + break;
>>>> + case SPMI_VIB:
>>>> + mask = SPMI_VIB_DRV_LEVEL_MASK;
>>>> + shift = SPMI_VIB_DRV_SHIFT;
>>>> + break;
>>>> + case SPMI_VIB_GEN2:
>>>> + mask = SPMI_VIB_GEN2_DRV_MASK;
>>>> + shift = SPMI_VIB_GEN2_DRV_SHIFT;
>>>> + break;
>>>> + default:
>>>> + return -EINVAL;
>>> Could you please move the switch to the previous patch? Then it would
>>> be more obvious that you are just adding the SPMI_VIB_GEN2 here.
>>>
>>> Other than that LGTM.
>>
>> Sure, I can move the switch to the previous refactoring patch.
>
> Actually, the idea of having a const "reg" or "chip", etc. structure is
> to avoid this kind of runtime checks based on hardware type and instead
> use common computation. I believe you need to move mask and shift into
> the chip-specific structure and avoid defining hw_type.
>
> Thanks.
Hi Dmitry,
The v7 changes have been pending for a while, I am not sure if you are
still insist on this. As I explained, I actually did it this way in v2
and it got updated to this by following other comments.
Can you respond and tell me if you prefer changes similar to v2? I can
update and push v8 by following your suggestion.
v7:
https://lore.kernel.org/linux-arm-msm/[email protected]/
v2:
https://lore.kernel.org/linux-arm-msm/[email protected]/
Thanks
>
Hi Fenglin,
On Thu, Mar 28, 2024 at 02:52:32PM +0800, Fenglin Wu wrote:
>
>
> On 2023/10/1 0:17, Dmitry Torokhov wrote:
> > On Mon, Sep 25, 2023 at 10:54:45AM +0800, Fenglin Wu wrote:
> > >
> > >
> > > On 9/24/2023 3:07 AM, Dmitry Baryshkov wrote:
> > > > > +
> > > > > + switch (vib->data->hw_type) {
> > > > > + case SSBI_VIB:
> > > > > mask = SSBI_VIB_DRV_LEVEL_MASK;
> > > > > shift = SSBI_VIB_DRV_SHIFT;
> > > > > + break;
> > > > > + case SPMI_VIB:
> > > > > + mask = SPMI_VIB_DRV_LEVEL_MASK;
> > > > > + shift = SPMI_VIB_DRV_SHIFT;
> > > > > + break;
> > > > > + case SPMI_VIB_GEN2:
> > > > > + mask = SPMI_VIB_GEN2_DRV_MASK;
> > > > > + shift = SPMI_VIB_GEN2_DRV_SHIFT;
> > > > > + break;
> > > > > + default:
> > > > > + return -EINVAL;
> > > > Could you please move the switch to the previous patch? Then it would
> > > > be more obvious that you are just adding the SPMI_VIB_GEN2 here.
> > > >
> > > > Other than that LGTM.
> > >
> > > Sure, I can move the switch to the previous refactoring patch.
> >
> > Actually, the idea of having a const "reg" or "chip", etc. structure is
> > to avoid this kind of runtime checks based on hardware type and instead
> > use common computation. I believe you need to move mask and shift into
> > the chip-specific structure and avoid defining hw_type.
> >
> > Thanks.
>
> Hi Dmitry,
>
> The v7 changes have been pending for a while, I am not sure if you are still
> insist on this. As I explained, I actually did it this way in v2 and it got
> updated to this by following other comments.
>
> Can you respond and tell me if you prefer changes similar to v2? I can
> update and push v8 by following your suggestion.
>
> v7: https://lore.kernel.org/linux-arm-msm/[email protected]/
>
> v2: https://lore.kernel.org/linux-arm-msm/[email protected]/
Yes, I believe what you had in v2 was better, and Dmitry Baryshkov's
comments on v2 were also great.
You can have 2 styles of code - you have a hw type for each regulator
and then use it to do conditional logic in the code. If you do it this
way you and you need to add a new device type or model you have to go
through the code and validate all the checks. Or you could have a
structure that is defined flexibly enough to cover all existing
permutations, and you rely on the data in it to control the behavior.
You should not mix the 2 styles, as this just makes the code more
confusing.
Thanks.
--
Dmitry