| Message ID | 20240401-pm8xxx-vibrator-new-design-v8-3-6f2b8b03b4c7@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for vibrator in multiple PMICs | expand |
On 4/1/24 10:38, Fenglin Wu via B4 Relay 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> > --- [...] > > -#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 VIB_MAX_LEVEL_mV(vib) (vib->drv2_addr ? (3544) : (3100)) You shouldn't need the additional inside parentheses Also, is this really a good discriminator for the voltage ranges? Do *all* PMIC vibrators with a drv2_addr operate within this range? If not, consider a struct field here > +#define VIB_MIN_LEVEL_mV(vib) (vib->drv2_addr ? (1504) : (1200)) > +#define VIB_MAX_LEVELS(vib) (VIB_MAX_LEVEL_mV(vib) - VIB_MIN_LEVEL_mV(vib)) If the ranges are supposed to be inclusive, this is off-by-one. But looking at the driver, it seems like MIN_LEVEL may be "off"? I'm not sure though. Either way, this would be a separate fix. [...] > +static struct pm8xxx_regs pmi632_regs = { > + .enable_offset = 0x46, > + .enable_mask = BIT(7), > + .drv_offset = 0x40, > + .drv_mask = 0xFF, GENMASK(7, 0) > + .drv_shift = 0, > + .drv2_offset = 0x41, > + .drv2_mask = 0x0F, GENMASK(3, 0) [...] > > + if (regs->drv2_mask) { > + if (on) > + val = (vib->level << regs->drv2_shift) & regs->drv2_mask; > + else > + val = 0; > + rc = regmap_write(vib->regmap, vib->drv2_addr, val); Are you purposefuly zeroing out the other bits? If yes, consider regmap_write_bits here If not, consider regmap_update_bits here > + if (rc < 0) > + return rc; Ignore regmap_r/w errors, these mean a complete failure of the API and we don't generally assume MMIO accesses can fail Unless SPMI is known to have issues here > + } > + > if (regs->enable_mask) > rc = regmap_update_bits(vib->regmap, vib->enable_addr, > regs->enable_mask, on ? ~0 : 0); > @@ -114,19 +141,22 @@ 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 /= 100; > + vib->level = ((VIB_MAX_LEVELS(vib) * vib->speed) / MAX_FF_SPEED) + mult_frac() > + VIB_MIN_LEVEL_mV(vib); vib->level = VIB_MIN_LEVEL_mV; vib->level += the other thing for readability? > } else { > vib->active = false; > - vib->level = VIB_MIN_LEVEL_mV / 100; > + vib->level = VIB_MIN_LEVEL_mV(vib); > + > } > > + if (!vib->drv2_addr) > + vib->level /= 100; Maybe this could be moved to pm8xxx_vib_set() instead > + > pm8xxx_vib_set(vib, vib->active); > } > > @@ -202,7 +232,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; It would be nice to preserve a newline between assignments and rw functions here Thanks for working on this! Konrad
Hi Konrad, On 4/11/2024 2:10 AM, Konrad Dybcio wrote: > > >> -#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 VIB_MAX_LEVEL_mV(vib) (vib->drv2_addr ? (3544) : (3100)) > > You shouldn't need the additional inside parentheses > > Also, is this really a good discriminator for the voltage ranges? Do *all* > PMIC vibrators with a drv2_addr operate within this range? If not, consider > a struct field here > Currently, yes, all PMIC vibrators with a drv2_addr (PMI632, PM7250B, PM7325B, PM7550BA) operate within the same range because they are the same type. > >> +#define VIB_MIN_LEVEL_mV(vib) (vib->drv2_addr ? (1504) : (1200)) >> +#define VIB_MAX_LEVELS(vib) (VIB_MAX_LEVEL_mV(vib) - >> VIB_MIN_LEVEL_mV(vib)) > > If the ranges are supposed to be inclusive, this is off-by-one. But looking > at the driver, it seems like MIN_LEVEL may be "off"? I'm not sure though. > > Either way, this would be a separate fix. > [...] The voltage range [1504, 3544] for the new SPMI vibrator is inclusive. I checked the voltage range [1200 3100] for PM8916 SPMI vibrator is also inclusive. I couldn't find any document to confirm if the SSBI vibrator but I assume it is the same as PM8916. I will make change before the series to address it. Thanks for reviewing the changes! Fenglin
diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c index 3b6a2e949f30..59548cd9331c 100644 --- a/drivers/input/misc/pm8xxx-vibrator.c +++ b/drivers/input/misc/pm8xxx-vibrator.c @@ -12,9 +12,9 @@ #include <linux/regmap.h> #include <linux/slab.h> -#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 VIB_MAX_LEVEL_mV(vib) (vib->drv2_addr ? (3544) : (3100)) +#define VIB_MIN_LEVEL_mV(vib) (vib->drv2_addr ? (1504) : (1200)) +#define VIB_MAX_LEVELS(vib) (VIB_MAX_LEVEL_mV(vib) - VIB_MIN_LEVEL_mV(vib)) #define MAX_FF_SPEED 0xff @@ -25,6 +25,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; }; @@ -44,6 +47,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 = 0xFF, + .drv_shift = 0, + .drv2_offset = 0x41, + .drv2_mask = 0x0F, + .drv2_shift = 8, + .drv_en_manual_mask = 0, +}; + /** * struct pm8xxx_vib - structure to hold vibrator data * @vib_input_dev: input device supporting force feedback @@ -52,6 +67,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 @@ -64,6 +80,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; @@ -92,6 +109,16 @@ 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(vib->regmap, vib->drv2_addr, val); + if (rc < 0) + return rc; + } + if (regs->enable_mask) rc = regmap_update_bits(vib->regmap, vib->enable_addr, regs->enable_mask, on ? ~0 : 0); @@ -114,19 +141,22 @@ 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 /= 100; + vib->level = ((VIB_MAX_LEVELS(vib) * vib->speed) / MAX_FF_SPEED) + + VIB_MIN_LEVEL_mV(vib); } else { vib->active = false; - vib->level = VIB_MIN_LEVEL_mV / 100; + vib->level = VIB_MIN_LEVEL_mV(vib); + } + if (!vib->drv2_addr) + vib->level /= 100; + pm8xxx_vib_set(vib, vib->active); } @@ -202,7 +232,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); if (error < 0) @@ -256,6 +286,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);