Messages in this thread | | | Date | Wed, 10 Apr 2024 20:10:42 +0200 | Subject | Re: [PATCH v8 3/3] input: pm8xxx-vibrator: add new SPMI vibrator support | From | Konrad Dybcio <> |
| |
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
| |