Re: [PATCH v8 3/3] input: pm8xxx-vibrator: add new SPMI vibrator support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux