| Message ID | 20240401-pm8xxx-vibrator-new-design-v8-3-6f2b8b03b4c7@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| 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
Hi Konrad, On 4/11/2024 2:10 AM, Konrad Dybcio wrote: > > >> + 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 > Sorry, forgot to reply on this comment. Yes, SPMI transaction would fail (even with very low odds) on some boards if the layout of SPMI lines is not good enough. I'd like to keep the consistence since the whole driver also checks the regmap_r/w errors.
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);