Re: [RESEND PATCH v6 1/3] input: pm8xxx-vib: refactor to easily support new SPMI vibrator

From: Dmitry Baryshkov
Date: Sat Sep 23 2023 - 15:06:09 EST

On Fri, 22 Sept 2023 at 11:38, Fenglin Wu <quic_fenglinw@xxxxxxxxxxx> 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 <quic_fenglinw@xxxxxxxxxxx>
> ---
> 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", &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