On 7/25/2023 1:52 PM, Krzysztof Kozlowski wrote:
On 25/07/2023 07:41, Fenglin Wu 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 introducing the HW type
terminology and contain the register offsets/masks/shifts in reg_filed
data structures corresponding to each vibrator type, and the base address
value defined in 'reg' property will be added for SPMI vibrators.
Signed-off-by: Fenglin Wu <quic_fenglinw@xxxxxxxxxxx>
---
drivers/input/misc/pm8xxx-vibrator.c | 130 ++++++++++++++++-----------
1 file changed, 77 insertions(+), 53 deletions(-)
diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
index 04cb87efd799..77bb0018d4e1 100644
--- a/drivers/input/misc/pm8xxx-vibrator.c
+++ b/drivers/input/misc/pm8xxx-vibrator.c
@@ -12,36 +12,36 @@
#include <linux/regmap.h>
#include <linux/slab.h>
+#define SSBI_VIB_DRV_EN_MANUAL_MASK 0xfc
+#define SSBI_VIB_DRV_LEVEL_MASK 0xf8
+#define SSBI_VIB_DRV_SHIFT 3
+#define SPMI_VIB_DRV_LEVEL_MASK 0xff
+#define SPMI_VIB_DRV_SHIFT 0
+
#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 pm8xxx_vib_type {
+ SSBI_VIB,
+ SPMI_VIB_GEN1,
+};
- unsigned int drv_addr;
- unsigned int drv_mask;
- unsigned int drv_shift;
- unsigned int drv_en_manual_mask;
+enum {
+ VIB_DRV_REG,
+ VIB_EN_REG,
+ VIB_MAX_REG,
};
-static const struct pm8xxx_regs pm8058_regs = {
- .drv_addr = 0x4A,
- .drv_mask = 0xf8,
- .drv_shift = 3,
- .drv_en_manual_mask = 0xfc,
+static struct reg_field ssbi_vib_regs[VIB_MAX_REG] = {
Change from const to non-const is wrong. How do you support multiple
devices? No, this is way too fragile now.
The register definition is no longer used as the match data, hw_type is
used.
The last suggestion was getting the register base address from the DT
and it has to be added into the offset of SPMI vibrator registers
(either in the previous hard-coded format or the later the reg_filed
data structure), so it's not appropriated to make it constant.
I don't understand this question: "How do you support multiple devices?"
For SSBI vibrator, since all the registers are fixed, and I would assume
that there is no chance to support multiple vibrator devices on the same
SSBI bus. If they are not on the same bus, the regmap device will be
different while the registers definition is the same, and we are still
able to support multiple devices, right?
The similar story for SPMI vibrators and it can support multiple devices
if they are located on different SPMI bus, or even if they are on the
same SPMI bus but just having different SID or PID.
...
/*
* pmic vibrator supports voltage ranges from 1.2 to 3.1V, so
@@ -168,38 +166,65 @@ static int pm8xxx_vib_probe(struct platform_device *pdev)
{
struct pm8xxx_vib *vib;
struct input_dev *input_dev;
- int error;
+ struct device *dev = &pdev->dev;
+ struct regmap *regmap;
+ struct reg_field *regs;
+ int error, i;
unsigned int val;
- const struct pm8xxx_regs *regs;
+ u32 reg_base;
- vib = devm_kzalloc(&pdev->dev, sizeof(*vib), GFP_KERNEL);
+ vib = devm_kzalloc(dev, sizeof(*vib), GFP_KERNEL);
Not really related. Split cleanup from new features.
Okay, will keep as it is.
if (!vib)
return -ENOMEM;
- vib->regmap = dev_get_regmap(pdev->dev.parent, NULL);
- if (!vib->regmap)
+ regmap = dev_get_regmap(dev->parent, NULL);
+ if (!regmap)
return -ENODEV;
- input_dev = devm_input_allocate_device(&pdev->dev);
+ input_dev = devm_input_allocate_device(dev);
if (!input_dev)
return -ENOMEM;
INIT_WORK(&vib->work, pm8xxx_work_handler);
vib->vib_input_dev = input_dev;
- regs = of_device_get_match_data(&pdev->dev);
+ vib->hw_type = (enum pm8xxx_vib_type)of_device_get_match_data(dev);
- /* operate in manual mode */
- error = regmap_read(vib->regmap, regs->drv_addr, &val);
+ regs = ssbi_vib_regs;
+ if (vib->hw_type != SSBI_VIB) {
+ error = fwnode_property_read_u32(dev->fwnode, "reg", ®_base);
+ if (error < 0) {
+ dev_err(dev, "Failed to read reg address, rc=%d\n", error);
+ return error;
+ }
+
+ if (vib->hw_type == SPMI_VIB_GEN1)
+ regs = spmi_vib_gen1_regs;
+
+ for (i = 0; i < VIB_MAX_REG; i++)
+ if (regs[i].reg != 0)
+ regs[i].reg += reg_base;
+ }
+
+ error = devm_regmap_field_bulk_alloc(dev, regmap, vib->r_fields, regs, VIB_MAX_REG);
if (error < 0)
+ {
That's not a Linux coding style.
Please run scripts/checkpatch.pl and fix reported warnings. Some
warnings can be ignored, but the code here looks like it needs a fix.
Feel free to get in touch if the warning is not clear.
+ dev_err(dev, "Failed to allocate regmap failed, rc=%d\n", error);
No need to print errors on allocation failures.
Will fix it.
Thanks
return error;
+ }
- val &= regs->drv_en_manual_mask;
- error = regmap_write(vib->regmap, regs->drv_addr, val);
+ error = regmap_field_read(vib->r_fields[VIB_DRV_REG], &val);
if (error < 0)
return error;
Best regards,
Krzysztof
