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.
...
>
> /*
> * 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.
> 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.
> 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
