On 4/1/24 10:38, Fenglin Wu via B4 Relay wrote:
From: Fenglin Wu <quic_fenglinw@xxxxxxxxxxx>
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@xxxxxxxxxxx>
---
[...]
-#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
